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

Update describe_pipeline to return more information about pipeline, including id for ensemble #1909

Merged
merged 19 commits into from Mar 4, 2021

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Mar 1, 2021

Closes #1872

  • Fix bug where ensemble was being added to IterativeAlgorithm._best_pipeline_info
  • Fix pipeline.describe() not having return_dict parameter

@angela97lin angela97lin self-assigned this Mar 1, 2021
@angela97lin angela97lin added this to the Sprint 2021 Feb B milestone Mar 1, 2021
@@ -154,11 +154,11 @@ def get_component(self, name):
"""
return self._component_graph.get_component(name)

def describe(self):
def describe(self, return_dict=False):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was missing return_dict parameter, even though it was in docstrings :p

assert component.name in out

@pytest.mark.parametrize("is_linear", [True, False])
def test_describe(is_linear, caplog, logistic_regression_binary_pipeline_class, nonlinear_binary_pipeline_class):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combined test_describe and test_describe_nonlinear

@@ -548,32 +548,23 @@ def test_indexing(X_y_binary, logistic_regression_binary_pipeline_class):
clf[:1]


def test_describe(caplog, logistic_regression_binary_pipeline_class):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combined test_describe and test_describe_nonlinear and test_describe_fitted and test_describe_nonlinear_fitted

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@@ -408,7 +408,6 @@ class DummyEstimator(Estimator):
assert pipeline.parameters == new_pipeline.parameters
for component, new_component in zip(pipeline._component_graph, new_pipeline._component_graph):
assert isinstance(new_component, type(component))
assert pipeline.describe() == new_pipeline.describe()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually just asserts None == None lol... also doesn't make sense to test equality via describe though (name is different), so no need for this test.

@@ -154,6 +154,7 @@ def test_iterative_algorithm_results(mock_stack, ensembling_value, dummy_binary_
random_seeds_the_same = [(estimator.pipeline.random_seed == algo.random_seed)
for estimator in estimators_used_in_ensemble]
assert all(random_seeds_the_same)
assert ModelFamily.ENSEMBLE not in algo._best_pipeline_info
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirms we don't add ensemble to _best_pipeline_info

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 I think this is great! I think the only thing blocking merge is that this implementation always returns the ids for the latest ensemble and not the ensemble that was asked for.

evalml/automl/automl_search.py Outdated Show resolved Hide resolved
@@ -548,32 +548,23 @@ def test_indexing(X_y_binary, logistic_regression_binary_pipeline_class):
clf[:1]


def test_describe(caplog, logistic_regression_binary_pipeline_class):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@angela97lin angela97lin removed the request for review from chukarsten March 2, 2021 23:02
@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #1909 (f909d77) into main (c8a029a) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #1909     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         267      267             
  Lines       21664    21699     +35     
=========================================
+ Hits        21658    21693     +35     
  Misses          6        6             
Impacted Files Coverage Δ
evalml/tests/pipeline_tests/test_pipeline_utils.py 100.0% <ø> (ø)
...lml/automl/automl_algorithm/iterative_algorithm.py 100.0% <100.0%> (ø)
evalml/automl/automl_search.py 100.0% <100.0%> (ø)
evalml/pipelines/pipeline_base.py 100.0% <100.0%> (ø)
evalml/tests/automl_tests/test_automl.py 100.0% <100.0%> (ø)
...lml/tests/automl_tests/test_iterative_algorithm.py 100.0% <100.0%> (ø)
evalml/tests/pipeline_tests/test_pipelines.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 c8a029a...f909d77. Read the comment docs.

Copy link
Contributor

@bchen1116 bchen1116 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 left a question for my own understanding, but nothing blocking.

evalml/automl/automl_search.py Outdated 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 Thanks for fixing! I think this is great!!

@angela97lin angela97lin merged commit d0b6101 into main Mar 4, 2021
@angela97lin angela97lin deleted the 1872_describe branch March 4, 2021 17:59
@dsherry dsherry mentioned this pull request Mar 11, 2021
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.

Update describe_pipeline to return more information about pipeline, including id for ensemble
3 participants