Skip to content

Remove Decision Tree and CatBoost Estimators from AutoML search #4205

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

Merged
merged 8 commits into from
Jun 14, 2023

Conversation

christopherbunn
Copy link
Contributor

@christopherbunn christopherbunn commented Jun 8, 2023

Resolves #4207, Resolves #4208

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #4205 (8b4245e) into main (d95ba3e) will decrease coverage by 0.0%.
The diff coverage is 93.4%.

@@           Coverage Diff           @@
##            main   #4205     +/-   ##
=======================================
- Coverage   99.7%   99.7%   -0.0%     
=======================================
  Files        349     349             
  Lines      38229   38229             
=======================================
- Hits       38111   38109      -2     
- Misses       118     120      +2     
Impacted Files Coverage Δ
...lml/tests/automl_tests/test_iterative_algorithm.py 100.0% <ø> (ø)
evalml/utils/gen_utils.py 99.3% <ø> (ø)
evalml/tests/pipeline_tests/test_pipelines.py 99.9% <50.0%> (ø)
evalml/tests/automl_tests/test_automl.py 99.5% <100.0%> (-<0.1%) ⬇️
.../automl_tests/test_automl_search_classification.py 96.4% <100.0%> (ø)
evalml/tests/pipeline_tests/test_pipeline_utils.py 99.6% <100.0%> (ø)
evalml/tests/utils_tests/test_logger.py 100.0% <100.0%> (ø)

@christopherbunn christopherbunn force-pushed the TML-7576_Remove_Estimators branch 2 times, most recently from ed95385 to 12cbba7 Compare June 13, 2023 15:46
@christopherbunn christopherbunn force-pushed the TML-7576_Remove_Estimators branch from 12cbba7 to 7f4865f Compare June 13, 2023 15:47
@christopherbunn christopherbunn marked this pull request as ready for review June 13, 2023 15:58
Copy link
Collaborator

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -115,9 +113,9 @@ def test_all_estimators(
is_using_conda,
):
if is_using_conda:
n_estimators = 17
n_estimators = 13
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this typically gets covered with our existing CI right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't believe so

@@ -1,6 +1,7 @@
{
"cells": [
{
"attachments": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why these got added here and removed from timeseries.ipynb?

Copy link
Contributor Author

@christopherbunn christopherbunn Jun 14, 2023

Choose a reason for hiding this comment

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

Ah, I think it's thrown in when you open the ipynb in VSCode. I thought I manually deleted some of these before I pushed. I'll take these added ones out and keep the removed ones in timeseries.ipynb!

@@ -4,6 +4,7 @@ Release Notes
* Enhancements
* Fixes
* Changes
* Remove Decision Tree and CatBoost Estimators from AutoML search :pr:`4205`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Remove -> Removed 😁

Also, thoughts on classifying this as a breaking change, since it's a pretty major removal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add another entry to point this out!

@christopherbunn christopherbunn enabled auto-merge (squash) June 14, 2023 15:38
@christopherbunn christopherbunn merged commit 6c5e9c8 into main Jun 14, 2023
@christopherbunn christopherbunn deleted the TML-7576_Remove_Estimators branch June 14, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove DecisionTree for all Problem Types Remove CatBoost for all Problem Types
3 participants