Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement Metadata for SMAC enabling Multi-Fidelity #771
base: main
Are you sure you want to change the base?
Implement Metadata for SMAC enabling Multi-Fidelity #771
Changes from 4 commits
548af15
36ac67a
4cc133b
5ab03c8
bfd2a42
16208f4
938f8f0
81d6d56
bf2f3cc
1686c7c
d263613
6766b8d
bae1763
53af62b
81e8bb0
dcff9cc
50ef16c
c32bd67
2b15694
574b8cc
3d4c055
41ee533
cfa936a
abd3eb6
e0ac571
c1e0845
9234599
054fce3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
These
Tuples
are getting a little large and hard to read (recall a previous version of this PR where the order of them was mistakenly swapped at one point).Think we discussed creating a NamedTuple or small DataClass for them instead so that they can be accessed by name in order to make it more readable.
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.
If you want I can do this in this PR, or another follow up PR
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.
I think a predecessor PR would be better. Much like we did with adding the metadata args and named args first.
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 is a pretty weird pattern. If we wanted to add more types in the future those kwargs wouldn't work.
What we've done elsewhere is
OptimizerType
in order to make sure that all optimizers get a test added (or explicitly excluded) whenever we add new optimizer backends.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.
I have moved the kwargs into the tuple which fixes the kwargs problem.
I find switching over unsupported optimizers messy since it means, in this case, we should skip the entire test as you have above, unless we are testing SMAC. There are other places where we test to make sure that using metadata throws exceptions, so without metadata support there is nothing to test.
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.
But when we add that functionality to another optimizer it will be very easy to forget to enable related testing support, so I'd rather add that now as it will be easier to search for what we need to enable.
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.
Won't it be the same either way? One requires adding a line, the other requires moving a line. I make this more similar to other tests, but I don't think it changes much either way.