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
Add configspace conversion #832
Add configspace conversion #832
Changes from all commits
3883fc2
e2dbae9
fa2663d
4c6811d
442c95e
061708d
acd5a46
12c656b
746970b
3f77f71
674c84e
6bf7518
a14d8c6
64555ed
c7b5a46
c9136fd
18f7395
72002cc
c083bff
8c7dac9
a5a4ac6
e4f6c56
5b8a2c0
107e5bd
47216cc
fd58aa1
8e8b47b
a8f0425
8abd4fb
eb57c58
fec66c3
ebc9886
0087ef8
016a6d2
5ff9d8e
59bdad2
0709f8d
17af4ce
38c1ae5
d818079
534ab5e
192f31c
a104255
f60858a
37c580e
65b6f7d
a14a47b
0a99146
9de07ad
5648f03
06adba3
bd9680f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't really see the point of having a
SpaceConverter
. Is it to force an external library to provide conversion functions functions for the given types of spaces?Also, this generic signature isn't really respected by the
ToConfigSpace
class, right? Since it doesn't supportFidelity
-type spaces, it returnsNone
in that case, and so it becomes aSpaceConverter[Optional[HyperParameter]]
!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.
bump
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 guess this answers my first question. However, it's clear from the
fidelity
example that you actually don't want to enforce that every type of dimension has to be supported by subclasses. Therefore, it's not actually an interface, if all its members are optional. It's only use, as far as I can see, is to be a list of names of methods that "can" be implemented by subclasses. It also doesn't have any state.At this point, why not just let the user code create a function that does what they want to do, e.g. serialize / print / do something for the types of
Dimension
of Orion they want to support?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.
To be fair the fidelity is the only exception. I don't see how the conversions could be usable if they do not support all other type of dimensions. I think the abstract class is the clearest way to express the interface expected for contributors. The only thing needed would be to clarify the exception for the fidelity.
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 essentially a singledispatch callable in disguise: You have handlers for each type below, and you dispatch based on the "type" of the argument, indirectly.
However, having an implicit dependency on the name of the dimension classes is very brittle, and hard to keep track of. Could you make this more explicit somehow?
I also don't think this should be inside a class, since there isn't (and most likely won't be) any state.
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.
Also, what happens if a new type of dimension is added, and there is no method with that name? This will raise an
AttributeError
, but you'd ideally like aNotImplementedError
with an informative message.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 we make it an abstract class then non-implemented methods would already raise an informative message during code development. It's true there is no state so making it a class isn't super useful for functionality's sake but I think it is nevertheless useful to make the interface more clear. The other solution would be to have a module for these functions only (without the methods for the conversion the other way around).
In any case, we should make it such that if the dev forgets to write some methods it fails with a clear message at execution or some tests automatically detects this.
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.
Actually, this should use the property
type
to work with transformed dimensions. Looks like we should also have tests for converting transformed spaces.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.
As long as:
then I don't have a problem with using an ABC and marking all the methods with
@abstractmethod
.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.
Since the abstract base class would only have static methods, this is actually a good use-case for Protocols:
This way, if you have a module like so:
The procol can be used just like the abstract base class, and has the benefit of allowing type checkers to also check against a module:
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 believe this should be removed, see below.
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 incoherent with the type signature of the
SpaceConverter[HyperParameter]
class.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.
bump
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 suggest moving this logic to a
Space
handler ofto_configspace
: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.
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 strongly suggest making this a singedispatch callable like you did with to_oriondim, and cutting-pasting the methods of the
ToConfigSpace
class into handler functions for each supported dimension type below.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 should be the "root" of the singledispatch callable, and just return a
NotImplementedError
for the given argument.