google / google/ml_collections

FieldReference op assumes type does not change when it does

Open
#18 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
1k
Forks
49
PR merge metrics
No merged PRs in 30d

Description

Hi, I am trying to do some more complicated configuration file setups which include putting together a couple of lazy ints to a lazy tuple. The following script is an example how I can get it to work. However the resulting `FieldReference` object stores a tuple but thinks it's storing an int.

```python
from ml_collections import ConfigDict, FieldReference
from ml_collections.config_dict import _Op

if __name__ == "__main__":
a = FieldReference(None, int)
b = FieldReference(None, int)

a_tuple = FieldReference(a, op=_Op(lambda x : (x,), ()))
b_tuple = FieldReference(b, op=_Op(lambda x : (x,), ()))
c = a_tuple + b_tuple

a.set(1)
b.set(2)

print(f"a_tuple: {a_tuple.get()}")
print(f"b_tuple: {b_tuple.get()}")
print(f"c: {c.get()}")

print(f"a_tuple type: {a_tuple._field_type}")
print(f"b_tuple type: {b_tuple._field_type}")
print(f"c type: {c._field_type}")
```

This outputs

```
a_tuple: (1,)
b_tuple: (2,)
c: (1, 2)
a_tuple type:
b_tuple type:
c type:
```

Which is probably a bug. This was run on python 3.10.8 and `ml_collections` 0.1.1.

Note that if we change the script to

```python
from ml_collections import ConfigDict, FieldReference
from ml_collections.config_dict import _Op

if __name__ == "__main__":
a = FieldReference(None, int)
b = FieldReference(None, int)

a_tuple = FieldReference(a, tuple, op=_Op(lambda x : (x,), ()))
b_tuple = FieldReference(b, tuple, op=_Op(lambda x : (x,), ()))
c = a_tuple + b_tuple

a.set(1)
b.set(2)

print(f"a_tuple: {a_tuple.get()}")
print(f"b_tuple: {b_tuple.get()}")
print(f"c: {c.get()}")

print(f"a_tuple type: {a_tuple._field_type}")
print(f"b_tuple type: {b_tuple._field_type}")
print(f"c type: {c._field_type}")
```

it throws an exception:

```
Traceback (most recent call last):
File "/home/andrius/repos/sde-sampling/tmp.py", line 8, in
a_tuple = FieldReference(a, tuple, op=_Op(lambda x : (x,), ()))
File "/home/andrius/env/lib/python3.10/site-packages/ml_collections/config_dict/config_dict.py", line 248, in __init__
self.set(default)
File "/home/andrius/env/lib/python3.10/site-packages/ml_collections/config_dict/config_dict.py", line 305, in set
raise TypeError('Reference is of type {} but should be of type {}'
TypeError: Reference is of type but should be of type
```

I think it would be better to change the behaviour as follows:

- If `field_type` is not specified when a `FieldReference` object is constructed, set it to the type of the argument `default` as it is done now.
- If `field_type` it is specified, then:
- If `op` is not specified and `field_type` is different from the type of `default`, raise an exception.
- If `op` is specified, raise an exception in `get` if the result of `op` is not the same as the `field_type` specified.

I'd be happy to file a PR if authors agree.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.