-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat(Optuna): Allow for parsing of Choice Nodes #290
Conversation
# filter all parameters given the made choices | ||
filtered_workspace = {k: v for k, v in workspace.items() if ( | ||
("__choice__" not in k) and | ||
(not any(c in k for c in delete_other_options)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line might need a second look, I had a bug a while back with something similar where essentially the c
was present in some other k
in another part of the pipeline that shouldn't have been effected by the given choice.
I'm on my phone so I cant write a concrete example but see the overall review comment for more
Looks really good, clean and well done, many thanks! I would ask that the actual sampling function is factored into its own function that is then tested a bit more as to it's sampling behaviour. This wasn't required for the other optimizers as I relied on them testing them own functionality, i.e. I dont want to test that other optimizers have their internals correct. However here, since we are the ones implementing the sampling procedure, might be good to have it factored out and testable. You can make it a private function of the module outside of the class as generally I don't think a user should be exposed to it. For the actual study object, you can choose any sampler that makes sense to you! Things to test are what was mentioned in the comment but also maybe that deeper more heirarchical pipelines work too. If you can correctly sample for this pipeline as well as a test for the aforementioned issue, I'd be happy to hit merge! If you can't get around to it, just let me know and I will try to finish it off! |
I fixed some stuff and added a OptunaSearchSpace class. It does the Choice parsing and has a sample_configuration() function. The main issue is probably still this filtering of the parameters for the unwanted choices. Now it just looks for ':{name_of_unwanted_choice}:'. Using the name of the node had trouble with flat vs. non-flat hierarchies and the Sequence node name which has some ?random elements?. So probably need to store the flat option in OptunaSearchSpace and do some smarter parsing for Sequence nodes. A pipeline where two Choice components have the same options is now probably not yet supported. Was just thinking if there are more options besides the two above to be taken into account for this filter logic. |
The random elements is usually just a thing of not giving a component a name. To do the configuring properly, I had to give them some identifier. In any case, happy to push this through once the test pass. In any case, I can turn the remaining thing into an issue so this can be merged! Very appreciative of your work! |
Thanks for the help! It's a really nice toolkit and codebase, was very glad when I found it 👍 The mypy crashes on my pre-commit, so running it locally.
|
Aye I'm aware of this issue, it's very annoying. I can't figure out what causes it but clearing cache seems to sometimes fix it -_- |
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Fixes #289.
Minimal Example / How should this PR be tested?
There are some extra tests, see
pytest -k optuna
.Any other comments?
Seems to be working. Will add new issues if I run into issues with it.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.