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

Delete static pipelines and refactor tests involving static pipelines #904

Merged
merged 33 commits into from
Jul 8, 2020

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Jul 1, 2020

Closes #845 by:

  • removing all_pipelines() and get_pipelines()
  • removing all statically-defined pipelines
  • removing pipelines/overview.ipynb notebook, since that was mostly used to highlight an example of a static pipeline (XGBoostBinaryPipeline)

Questions:

  • What tests should we add to offset all of the pipeline tests we're removing, especially if we're not using some pipelines in automl (and can potentially never use in pipeline)? Component tests? Are they enough?

@angela97lin angela97lin self-assigned this Jul 1, 2020
@angela97lin angela97lin added this to the July 2020 milestone Jul 1, 2020
@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #904 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
+ Coverage   99.81%   99.83%   +0.01%     
==========================================
  Files         197      168      -29     
  Lines        9168     8352     -816     
==========================================
- Hits         9151     8338     -813     
+ Misses         17       14       -3     
Impacted Files Coverage Δ
evalml/pipelines/__init__.py 100.00% <ø> (ø)
evalml/pipelines/classification/__init__.py 100.00% <ø> (ø)
...transformers/feature_selection/feature_selector.py 88.23% <ø> (+4.45%) ⬆️
evalml/pipelines/regression/__init__.py 100.00% <ø> (ø)
evalml/pipelines/utils.py 100.00% <ø> (ø)
evalml/tests/component_tests/test_components.py 100.00% <ø> (ø)
...l/tests/component_tests/test_xgboost_classifier.py 100.00% <ø> (ø)
...ml/tests/component_tests/test_xgboost_regressor.py 100.00% <ø> (ø)
evalml/tests/objective_tests/test_objectives.py 100.00% <ø> (+1.81%) ⬆️
evalml/utils/gen_utils.py 100.00% <ø> (ø)
... and 12 more

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 133c9bc...c4d320e. Read the comment docs.

@@ -452,13 +416,13 @@ def test_estimator_not_last(X_y_binary):
}
}

class MockLogisticRegressionBinaryPipeline(BinaryClassificationPipeline):
name = "Mock Logistic Regression Pipeline"
class MockBinaryClassificationPipelineWithoutEstimator(BinaryClassificationPipeline):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly related, but renamed when I was making a fixture for clarity.

@@ -6,15 +6,6 @@
class FeatureSelector(Transformer):
"""Selects top features based on importance weights"""

def get_indices(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed since it wasn't being used anywhere; perhaps this was some remnants from previous code?

@angela97lin angela97lin marked this pull request as ready for review July 6, 2020 19:18
Copy link
Contributor

@dsherry dsherry 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!! 👏 I left a suggestion on the tests you added, and some docs questions. But nothing blocking merge.

docs/source/changelog.rst Show resolved Hide resolved
docs/source/pipelines/components.ipynb Outdated Show resolved Hide resolved
evalml/tests/component_tests/test_catboost_classifier.py Outdated Show resolved Hide resolved
evalml/tests/pipeline_tests/test_pipelines.py Show resolved Hide resolved
evalml/tests/pipeline_tests/test_pipelines.py Show resolved Hide resolved
docs/source/pipelines/custom_pipelines.ipynb Show resolved Hide resolved
Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

@angela97lin Left some minor comments - nothing blocking though!

@angela97lin angela97lin merged commit 5b7afaa into master Jul 8, 2020
@angela97lin angela97lin deleted the 845_static_pipelines branch July 8, 2020 00:03
@dsherry dsherry mentioned this pull request Jul 16, 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
Development

Successfully merging this pull request may close these issues.

Delete static pipelines and refactor tests involving static pipelines
3 participants