Skip to content

Add Drop Columns Transformer pipeline parameters for DefaultAlgorithm#2945

Merged
jeremyliweishih merged 23 commits into
mainfrom
js_2939_drop_columns_default_algo
Oct 27, 2021
Merged

Add Drop Columns Transformer pipeline parameters for DefaultAlgorithm#2945
jeremyliweishih merged 23 commits into
mainfrom
js_2939_drop_columns_default_algo

Conversation

@jeremyliweishih
Copy link
Copy Markdown
Collaborator

Fixes #2939.

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 21, 2021

Codecov Report

Merging #2945 (f714be9) into main (c690662) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2945     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        307     307             
  Lines      29215   29244     +29     
=======================================
+ Hits       29124   29153     +29     
  Misses        91      91             
Impacted Files Coverage Δ
evalml/automl/automl_algorithm/automl_algorithm.py 100.0% <100.0%> (ø)
...valml/automl/automl_algorithm/default_algorithm.py 100.0% <100.0%> (ø)
...lml/automl/automl_algorithm/iterative_algorithm.py 100.0% <100.0%> (ø)
...valml/tests/automl_tests/test_default_algorithm.py 100.0% <100.0%> (ø)

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 c690662...f714be9. Read the comment docs.

@jeremyliweishih jeremyliweishih changed the title Move Drop Columns Transformer parameters to make_pipeline Add Drop Columns Transformer pipeline parameters for DefaultAlgorithm Oct 25, 2021
@jeremyliweishih jeremyliweishih marked this pull request as ready for review October 25, 2021 18:03
Copy link
Copy Markdown
Contributor

@eccabay eccabay left a comment

Choose a reason for hiding this comment

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

LGTM! Just one clarifying question about a copied line

Comment thread evalml/automl/automl_algorithm/automl_algorithm.py Outdated
Copy link
Copy Markdown
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think we might want to file an issue to consider how to refactor the chunk moved into the private function to be shared between both Algorithms. That might help us think more about the lift & shift modularity that we wanted in the Algorithm base.

next_batch.append(ensemble)
return next_batch

def _set_additional_pipeline_params(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like functionality from IterativeAlgorithm's _create_pipelines() function. Should we figure out how to refactor both these algorithm's to call the common function? This isn't blocking as the code chunk was already in there, but it would probably be worth an issue.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Filed #2970!

@jeremyliweishih jeremyliweishih merged commit 4dffea4 into main Oct 27, 2021
@chukarsten chukarsten mentioned this pull request Oct 27, 2021
@freddyaboulton freddyaboulton deleted the js_2939_drop_columns_default_algo branch May 13, 2022 15:23
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.

Drop Columns Transformer does not have columns to drop in DefaultAlgorithm

3 participants