-
Notifications
You must be signed in to change notification settings - Fork 87
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 'search_results' and 'search_order' #260
Conversation
Codecov Report
@@ Coverage Diff @@
## master #260 +/- ##
==========================================
+ Coverage 97.07% 97.08% +0.01%
==========================================
Files 95 95
Lines 2731 2742 +11
==========================================
+ Hits 2651 2662 +11
Misses 80 80
Continue to review full report at Codecov.
|
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.
looks good. proposed an api change to naming
evalml/models/auto_base.py
Outdated
@@ -59,6 +59,8 @@ def __init__(self, problem_type, tuner, cv, objective, max_pipelines, max_time, | |||
else: | |||
raise TypeError("max_time must be a float, int, or string. Received a {}.".format(type(max_time))) | |||
self.results = {} |
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.
maybe just init all at once?
self.results = {
'pipelines': {},
'search_order` []
}
also what if we called the key pipelines
instead of search_results
?
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.
@kmax12 I originally was going to call it pipelines but thought it would be misleading since it doesn't actually store the pipeline itself. Would pipeline_results
work?
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.
sure. i like that
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.
looks good. just one comment about noting the breaking change
@@ -6,6 +6,7 @@ Changelog | |||
* Enhancements | |||
* Added ability to create a plot of feature importances :pr:`133` | |||
* Added ROC and confusion matrix metrics and plot for classification problems and introduce PipelineSearchPlots class :pr:`242` | |||
* Enhanced AutoML results with search order :pr:`260` |
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 we should add a "breaking change" warning to the changelog that the results dictionary changed
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.
one comment about changelog, otherwise LGTM
docs/source/changelog.rst
Outdated
@@ -21,6 +22,12 @@ Changelog | |||
* Testing Changes | |||
* Added support for testing on Windows with CircleCI :pr:`226` | |||
* Added support for doctests :pr:`233` | |||
|
|||
**Breaking Changes** | |||
* `autoclassifier.results` and `autoregressor.results` now is a dictionary |
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.
should be AutoClassifier
and AutoRegressor
Made
self.results
inAutoBase
to handle more information we would want to hold. This PR addesself.results['search_results] and self.results['search_order']). Relates to #160 and #241. If more information is needed in the future we can extend
self.results`.