-
Notifications
You must be signed in to change notification settings - Fork 93
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
Easy api #255
Easy api #255
Conversation
I'll clean up things once there's some feedback :) |
Codecov Report
@@ Coverage Diff @@
## main #255 +/- ##
==========================================
+ Coverage 66.72% 67.64% +0.92%
==========================================
Files 17 24 +7
Lines 1665 1768 +103
==========================================
+ Hits 1111 1196 +85
- Misses 554 572 +18
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@nabenabe0928 @NeoChaos12 Feel free to also provide feedback on the API |
I really like how you can interact with configspace right now. I added examples in both readme and docs. |
Waiting to rebase onto #259 to fix errors in tests |
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.
Hey folks, I started having a look at this PR, but I have a few questions before I can continue looking further. In any case, it looks very nice and I like the new API very much.
One thing which came up earlier in my mind was that we have
Int, in contrast to float and categorical, is shortcut. Should we change it to Integer for consistency? |
Sound good. |
Well I don't think |
I had an offline discussion with @eddiebergman and we agreed to add the doctests back in. I will do a full review after these changes. |
@mfeurer I've updated to doctests again. Took a long ass while to debug why things weren't working but I've also included the changelog in this PR since it bumps the version number, theoretically this could be done in a seperate PR but I wanted to just get it done in one place. Seems there is also some tolerance needed for a test @renesass just a note when changing from
Assuming all tests checks now pass, I think it's good to go |
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.
Very minor, looks good to go :)
Extra few things done, only possible slowdown is the "docs" workflow, I hope that doesn't catch. There was a numerical thing to do with some floating point value and I hope it doesn't trigger |
This PR aims to
To facilitate automl/SMAC3#847, in which the user needs to know about ConfigSpace before they can even try SMAC, this PR allows basic ConfigSpace's to be built using a dict
Categorical
andOrdinal
wasn't possible as Categorical's state that they are indeterminate w.r.t. to having a set. This might be possible to solve by simply sorting the received set inCategorical
but outside of scope.Another thing in this PR is to provide a simpler interface for constructing hyperparameters.
These are type safe so Mypy correctly knows that
b
is anOrdinalHyperparamter
here, as well as for the rest of them.The two of these combine to form:
This also updates all documentation to reflect this (as well as update documentation in general)
You can run the following to download, build and view the docs:
Note that it has to unfortunately build the package first because Cython (grrr) needs to read the docstrings and generate something? That's why it's so long but the command mostly cleans up after itself, you just have to delete the
ConfigSpace
folder created once you're done. Might this might not work on Windows/Mac