Update `add_results` in IterativeAlgorithm to take in entire pipeline results dict #1891

angela97lin · 2021-02-24T19:45:27Z

codecov · 2021-02-24T20:43:19Z

Codecov Report

Merging #1891 (2640d5e) into main (f23f16b) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##             main    #1891     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         263      263             
  Lines       21285    21300     +15     
=========================================
+ Hits        21279    21294     +15     
  Misses          6        6

Impacted Files	Coverage Δ
evalml/automl/automl_algorithm/automl_algorithm.py	`100.0% <100.0%> (ø)`
...lml/automl/automl_algorithm/iterative_algorithm.py	`100.0% <100.0%> (ø)`
evalml/automl/automl_search.py	`100.0% <100.0%> (ø)`
evalml/tests/automl_tests/test_automl_algorithm.py	`100.0% <100.0%> (ø)`
...lml/tests/automl_tests/test_iterative_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 f23f16b...2640d5e. Read the comment docs.

angela97lin · 2021-02-25T16:01:26Z

evalml/tests/automl_tests/test_iterative_algorithm.py

@@ -346,3 +350,25 @@ def test_iterative_algorithm_pipeline_params_kwargs(dummy_binary_pipeline_classe

    next_batch = algo.next_batch()
    assert all([p.parameters['Mock Classifier'] == {"dummy_parameter": "dummy", "n_jobs": -1, "fake_param": "fake"} for p in next_batch])
+
+
+def test_iterative_algorithm_results_best_pipeline_info_id(dummy_binary_pipeline_classes, logistic_regression_binary_pipeline_class):


Blargh, I wonder if there's a better way to test this. Or if we need to, since this is just a private field for now anyways.
But currently, creating IterativeAlgo with two model family pipelines. Add results. Since only one round of results have been added, we know what's in _best_pipeline_info has to be the two pipelines (with ids 0 and 1). :)

If these IDs are to be used for ensembling how about we add coverage for this in the ensembling unit tests (make sure our ensembler keeps track of these IDs)? Not in the loop on whether that has been implemented or not, so we if we have that feature already we can add the test there if not we can flag to remember to do so when we add that feature.

I think it's fine to leave this in for now, if you want, as a quick sanity check. I the only thing I would question is do you need all the other assertions? It seems like they just clutter what it is you're trying to test, which is that the id's get assigned properly to the _best_pipeline_info. I'd think there are other tests to make sure, for example, that next_batch increments the batch_number ala L367. Just my two cents though, don't bother if you like it better.

@jeremyliweishih @chukarsten Thank you for the suggestions and thoughts!

The IDs are for ensembling, but our ensembler won't be keeping track of it--we decided that since ids are more of a concept for automl and not an individual pipeline, so we need to test that the IterativeAlgorithm class is updating accordingly. This test was a little meh since it only did it for the first batch. Updated the test s.t. the score is always decreasing, so we always try to update _best_pipeline_info, and asserted that the id is indeed always updating too.

Thank you for the suggestion to remove other asserts--they were definitely copy pasta asserts that we've just kept in each test but for the sake of this test, I think it looks much clearer without!

jeremyliweishih

Looks good from my end. IMO we should add coverage for the pipeline IDs in their intended use case (ensembling) especially since its a private field.

jeremyliweishih · 2021-02-25T16:20:39Z

evalml/tests/automl_tests/test_iterative_algorithm.py

@@ -346,3 +350,25 @@ def test_iterative_algorithm_pipeline_params_kwargs(dummy_binary_pipeline_classe

    next_batch = algo.next_batch()
    assert all([p.parameters['Mock Classifier'] == {"dummy_parameter": "dummy", "n_jobs": -1, "fake_param": "fake"} for p in next_batch])
+
+
+def test_iterative_algorithm_results_best_pipeline_info_id(dummy_binary_pipeline_classes, logistic_regression_binary_pipeline_class):


If these IDs are to be used for ensembling how about we add coverage for this in the ensembling unit tests (make sure our ensembler keeps track of these IDs)? Not in the loop on whether that has been implemented or not, so we if we have that feature already we can add the test there if not we can flag to remember to do so when we add that feature.

chukarsten

Nice, quick PR. Love it.

chukarsten · 2021-02-25T16:27:55Z

evalml/tests/automl_tests/test_iterative_algorithm.py

@@ -346,3 +350,25 @@ def test_iterative_algorithm_pipeline_params_kwargs(dummy_binary_pipeline_classe

    next_batch = algo.next_batch()
    assert all([p.parameters['Mock Classifier'] == {"dummy_parameter": "dummy", "n_jobs": -1, "fake_param": "fake"} for p in next_batch])
+
+
+def test_iterative_algorithm_results_best_pipeline_info_id(dummy_binary_pipeline_classes, logistic_regression_binary_pipeline_class):


I think it's fine to leave this in for now, if you want, as a quick sanity check. I the only thing I would question is do you need all the other assertions? It seems like they just clutter what it is you're trying to test, which is that the id's get assigned properly to the _best_pipeline_info. I'd think there are other tests to make sure, for example, that next_batch increments the batch_number ala L367. Just my two cents though, don't bother if you like it better.

ParthivNaresh

Looks great!

angela97lin · 2021-02-25T18:43:39Z

docs/source/release_notes.rst

@@ -3,7 +3,9 @@ Release Notes
 **Future Releases**
    * Enhancements
    * Fixes
+        * Added metaclass for time series pipelines and fix binary classification pipeline ``predict`` not using objective if it is passed as a named argument :pr:`1874`


Moving, oops hehe. Thanks @freddyaboulton!

init

0703280

angela97lin self-assigned this Feb 24, 2021

angela97lin added 2 commits February 24, 2021 15:22

fix tests

ce7737a

fix test

61cb11d

angela97lin added 4 commits February 24, 2021 19:51

add test

65a4f47

lint

3abd92e

Merge branch 'main' into 1873_dict

92a3ca5

cleanup docstr

1dc427f

angela97lin commented Feb 25, 2021

View reviewed changes

angela97lin marked this pull request as ready for review February 25, 2021 16:01

angela97lin requested review from dsherry, freddyaboulton, bchen1116, chukarsten, jeremyliweishih and ParthivNaresh February 25, 2021 16:01

jeremyliweishih approved these changes Feb 25, 2021

View reviewed changes

chukarsten approved these changes Feb 25, 2021

View reviewed changes

ParthivNaresh approved these changes Feb 25, 2021

View reviewed changes

cleanup

2640d5e

angela97lin commented Feb 25, 2021

View reviewed changes

angela97lin merged commit 7b400c8 into main Feb 25, 2021

angela97lin deleted the 1873_dict branch February 25, 2021 19:31

angela97lin added this to the Sprint 2021 Feb B milestone Mar 4, 2021

dsherry mentioned this pull request Mar 11, 2021

v0.20.0 #1961

Merged

Update `add_results` in IterativeAlgorithm to take in entire pipeline results dict #1891

Update `add_results` in IterativeAlgorithm to take in entire pipeline results dict #1891

angela97lin commented Feb 24, 2021

codecov bot commented Feb 24, 2021 •

edited

angela97lin Feb 25, 2021

jeremyliweishih Feb 25, 2021

chukarsten Feb 25, 2021

angela97lin Feb 25, 2021

jeremyliweishih left a comment

jeremyliweishih Feb 25, 2021

chukarsten left a comment

chukarsten Feb 25, 2021

ParthivNaresh left a comment

angela97lin Feb 25, 2021

Update add_results in IterativeAlgorithm to take in entire pipeline results dict #1891

Update add_results in IterativeAlgorithm to take in entire pipeline results dict #1891

Conversation

angela97lin commented Feb 24, 2021

codecov bot commented Feb 24, 2021 • edited

Codecov Report

angela97lin Feb 25, 2021

Choose a reason for hiding this comment

jeremyliweishih Feb 25, 2021

Choose a reason for hiding this comment

chukarsten Feb 25, 2021

Choose a reason for hiding this comment

angela97lin Feb 25, 2021

Choose a reason for hiding this comment

jeremyliweishih left a comment

Choose a reason for hiding this comment

jeremyliweishih Feb 25, 2021

Choose a reason for hiding this comment

chukarsten left a comment

Choose a reason for hiding this comment

chukarsten Feb 25, 2021

Choose a reason for hiding this comment

ParthivNaresh left a comment

Choose a reason for hiding this comment

angela97lin Feb 25, 2021

Choose a reason for hiding this comment

Update `add_results` in IterativeAlgorithm to take in entire pipeline results dict #1891

Update `add_results` in IterativeAlgorithm to take in entire pipeline results dict #1891

codecov bot commented Feb 24, 2021 •

edited