Refactor Config and Variables #142
Conversation
|
@Rykath (maybe also @krystophny) please also check the |
|
The |
Before refactoring the Variable configuration, spec['dtype'] was a string. ZeroMQ requires a string to be able to serialize the dtype for sending it to the Worker. With `__name__` we get a correct string which can be read by numpy correctly again. Problems maybe requiring further action: * The real error "TypeError: Object of type type is not JSON serializable" was not shown in the test * The error within the thread did not cause the pytest to fail
I just checked the |
| @@ -13,7 +13,7 @@ | |||
| - .hdf5 output of single runs with custom interface | |||
| """ | |||
|
|
|||
| from profit.config import Config | |||
| from profit.config import BaseConfig | |||
There was a problem hiding this comment.
It might have been easier to say
from profit.config import BaseConfig as Configbut this is cleaner anyways
| break | ||
|
|
||
|
|
||
| @RunnerConfig.register("local") |
There was a problem hiding this comment.
I would personally prefer it if the actual configurations are next to the relevant implementation. This would ensure that all relevant information of that component is just in one file.
But that's probably a discussion for another time...
| vars.append(Variable.create_from_str(k, (config['ntrain'], 1), v)) | ||
| else: | ||
| vars.append(Variable.create(**v)) | ||
| variables.add(vars) |
There was a problem hiding this comment.
That is a bit strange: While creating the BaseConfig the Variable objects were already built, but then replaced with a dictionary. Now the dictionary is converted into objects again.
I guess it's fine for now (as probably some other lines depend on the dictionary structure), but in the future we should consider completing the transformation into an object oriented config (see issue #143)
profit/util/variable_kinds.py
Outdated
| return cls.create(**v_dict) | ||
|
|
||
| @classmethod | ||
| def create(cls, name, kind, size, entries=(0, 1), value=np.array([[]]), dtype=np.float64): |
There was a problem hiding this comment.
Using np.array([[]]) as a default value is dangerous, as the same value would be shared across all Variable objects. (similar to how a list as a default value works) In this case it might just work anyways.
Also, why do you use create instead of just __init__? I'm just curious.
defaults.pyfile to store all default config values.Close warn user about unknown config options #110
handle_configmethods to the Config subclasses.Close Refactor class and config structure #132