Skip to content
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

AutoSearchBase: refactor pipeline evaluation into evaluate method #762

Merged
merged 3 commits into from May 11, 2020

Conversation

dsherry
Copy link
Collaborator

@dsherry dsherry commented May 9, 2020

Fixes #752, fixes #568

Update AutoSearchBase to have a separate evaluate method, to make it easy to evaluate pipeline scores. This will replace AutoSearchBase._do_iteration, but where the next pipeline and parameters are proposed in AutoSearchBase.search instead.

@codecov
Copy link

codecov bot commented May 9, 2020

Codecov Report

Merging #762 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #762   +/-   ##
=======================================
  Coverage   99.38%   99.38%           
=======================================
  Files         151      151           
  Lines        5529     5532    +3     
=======================================
+ Hits         5495     5498    +3     
  Misses         34       34           
Impacted Files Coverage Δ
...ents/estimators/classifiers/catboost_classifier.py 100.00% <ø> (ø)
evalml/automl/auto_search_base.py 97.53% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eee3ea6...2304757. Read the comment docs.

@dsherry dsherry force-pushed the ds_752_separate_evaluate branch from ed81f9d to 2304757 Compare May 11, 2020
@dsherry dsherry marked this pull request as ready for review May 11, 2020
@@ -20,7 +20,7 @@ class CatBoostClassifier(Estimator):
hyperparameter_ranges = {
"n_estimators": Integer(10, 1000),
"eta": Real(0, 1),
"max_depth": Integer(1, 16),
"max_depth": Integer(4, 10),
Copy link
Collaborator Author

@dsherry dsherry May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past we've had some trouble with the performance of the catboost pipeline (#568). I think I know why: the max tree depth here was too large. Catboost recommends a range of 4 to 10. I think training time grows nonlinearly with this parameter, meaning a depth of 16 will take a lot longer to run.

Why is this relevant to this PR: for convenience/clarity, this PR changed the order of calls to the RNG, here. This happened to mean that we run catboost in the lead scoring notebook with depth=16, whereas before we happened to not run catboost in that notebook. Catboost took 9min to train on that data with that depth. Updating the max to 10 has it training in a reasonable time. The specific change in this PR isn't critical, but once I figured out the problem I wanted to fix it.

Another argument for the perf tests being highly valuable to us! Not that we needed more evidence, haha.

Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

LGTM

@dsherry dsherry merged commit 87f9df4 into master May 11, 2020
2 checks passed
@dsherry dsherry deleted the ds_752_separate_evaluate branch May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants