-
Notifications
You must be signed in to change notification settings - Fork 86
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 Tuner base class #351
Add Tuner base class #351
Conversation
evalml/tuners/tuner.py
Outdated
|
||
|
||
class Tuner(ABC): | ||
"""Base Tuner 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.
Hmm, I should probably add an API example here for the doc
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.
By API example do you mean just part of the API reference? or one of the longer notebooks?
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.
Part of the API reference
Codecov Report
@@ Coverage Diff @@
## master #351 +/- ##
==========================================
+ Coverage 97.29% 97.36% +0.07%
==========================================
Files 102 104 +2
Lines 3180 3266 +86
==========================================
+ Hits 3094 3180 +86
Misses 86 86
Continue to review full report at Codecov.
|
5eea368
to
39f35ed
Compare
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 looks good to me other than needing tests to pass. One thing I wanted to point out was that it might be clearer to explicitly set random state in the tests to show that propose isn't completely random but it does get tedious and verbose.
In the future we should discuss some of these ideas:
- User parameterization of skopt: changing base estimator, optimizer, etc.
- Exploration of skopt settings: do we have the optimal default settings?
- Should we stick with or move away from add, propose?
"""Given two sets of numeric/str parameter lists, assert numerics are approx equal and strs are equal""" | ||
def separate_numeric_and_str(values): | ||
is_numeric = lambda val: isinstance(val, (int, float)) | ||
extract = lambda vals, invert: [el for el in vals if (invert ^ is_numeric(el))] |
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.
Don't think it is an issue but would this include boolean values as a "string"?
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.
Yes, this code wouldn't work for booleans. If we add those to our supported parameters, we'd have to update this. But we don't support boolean parameters right now, right? I couldn't find any--and we can always represent them as categorical, which may make more sense anyways
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.
Yup makes sense to me 👍
@jeremyliweishih nice, yeah agreed. #272 tracks the last question, of whether we should stick with I'll add fixing the random_state, why not. |
d58cd6e
to
db98a66
Compare
Wft, why is codecov failing on this PR?! That's weird 😅 |
e0d8594
to
3dd327e
Compare
pragma: no cover | ||
|
||
# Don't complain if tests don't hit defensive assertion code: | ||
raise NotImplementedError |
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 needed to avoid the covtests complaining about the abstract base class:
https://stackoverflow.com/questions/9202723/excluding-abstractproperties-from-coverage-reports
3dd327e
to
c09f4b4
Compare
@christopherbunn @jeremyliweishih this is ready to go, just needs review and approval! |
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.
We currently only have one tuner,
SKOptTuner
, but @christopherbunn is adding two more in #230 . This PR puts in an abstract base class to define the current interface.I'd like to consider changing/expanding the tuner API in #272 , but for now this will unblock #230 and provide more test coverage.