Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dictionary fields #289

Open
jneeven opened this issue Oct 25, 2021 · 4 comments
Open

Dictionary fields #289

jneeven opened this issue Oct 25, 2021 · 4 comments
Labels
feature New feature or request

Comments

@jneeven
Copy link
Contributor

jneeven commented Oct 25, 2021

Feature motivation

If you have multiple variants of some kind of component, e.g. multiple preprocessing functions, it would be very nice to store them all in a dictionary rather than having to define separate fields (which is error-prone especially as component classes get bigger, subclass other components etc)

(CC @CNugteren)

Feature description

Would be nice to be able to do something like this:

preprocessing: Dict[str, Preprocessing] = {
    "default": ComponentField(),
    "custom": ComponentField(),
}

Of course the fields would still have to be CLI overridable, e.g. MyComponent.preprocessing.default.some_attribute="value"

Feature implementation

I don't think this will necessarily be easy, but I also don't think it should be too difficult. We'll mainly have to make sure the config "paths" are handled correctly and the inheritance works properly. I'd hope this could be done in a day or so.

@jneeven jneeven added the feature New feature or request label Oct 25, 2021
@AdamHillier
Copy link
Contributor

I'm not quite sure I understand the motivation exactly — to be clear, you don't want the dict itself to be modifyable (i.e. don't want to be able to add new keys to the dict dynamically)?

@jneeven
Copy link
Contributor Author

jneeven commented Oct 27, 2021

to be clear, you don't want the dict itself to be modifyable (i.e. don't want to be able to add new keys to the dict dynamically)?

Being able to add keys would be great, but I'd already be very happy just to be able to CLI-modify the key-value pairs that are already provided in the code (for tuning purposes). With the example above, the current solution would be resort to something like

default_preprocessing = ...
custom_preprocessing = ...

Which is very much suboptimal and also doesn't allow specifying new "keys" through the CLI

@AdamHillier
Copy link
Contributor

I guess that's my question then: assuming that we don't allow key insertion, why would the current approach be suboptimal?

Another question I have is how subclassing would work with overriding field defaults: if B subclasses A but only wants to change the default for preprocessing["custom"], how would that work?

@jneeven
Copy link
Contributor Author

jneeven commented Oct 27, 2021

Another question I have is how subclassing would work with overriding field defaults: if B subclasses A but only wants to change the default for preprocessing["custom"], how would that work?

I'd say it wouldn't, if you change anything you'd need to change the entire thing. Of course being able to just add and change fields would be convenient, but it'd become a mess very quickly. I don't think we could support the case you describe without simultaneously removing support for changing the entire field (e.g. deleting keys), which seems a more important feature to have

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants