-
Notifications
You must be signed in to change notification settings - Fork 92
Preserve _ESTIMATOR_FAMILY_ORDER in IterativeAlgorithm #2850
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2850 +/- ##
=======================================
+ Coverage 99.0% 99.7% +0.8%
=======================================
Files 302 302
Lines 28626 28637 +11
=======================================
+ Hits 28315 28544 +229
+ Misses 311 93 -218
Continue to review full report at Codecov.
|
c75af28 to
01553db
Compare
|
Perf tests have been updated with results from a new default model family order. |
bchen1116
left a comment
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.
LGTM! I'm fine with changing the order to get generally-better model performance. I left a comment on removing the excess commented code, but the implementation looks great!
| estimators_in_first_batch | ||
| == linear_models + extra_dep_estimators + core_estimators | ||
| ) | ||
| # if problem_type == ProblemTypes.BINARY: |
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 these comments be removed?
chukarsten
left a comment
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 hold off on this one....
|
Sounds good @chukarsten ! When do you think would be a good time to merge this? |
chukarsten
left a comment
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.
Cool, cool. I think we can move forward with this. Are you intending to keep those commented if clauses? I think just tidy that up and we're gucci.
ca00283 to
ceb4175
Compare
|
@alteryx/evalml I re-ran the perf tests after merging in the latest main since it had been a while. Results from the previous analysis still hold. Anyone have any thoughts before I merge later today or tomorrow? |
ddc4221 to
4329182
Compare
4329182 to
ce12981
Compare
Pull Request Description
Fixes #2849
Controversial perf tests here: https://alteryx.atlassian.net/wiki/spaces/PS/pages/1061093724/2849+Preserve+Pipeline+Order 😂
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rstto include this pull request by adding :pr:123.