Added drop NaN component to some time series pipelines #3310

christopherbunn · 2022-02-08T17:00:26Z

Resolves #3259

codecov · 2022-02-08T17:07:44Z

Codecov Report

Merging #3310 (395383c) into main (2398f09) will increase coverage by 0.1%.
The diff coverage is 100.0%.

❗ Current head 395383c differs from pull request most recent head 826c79e. Consider uploading reports for the commit 826c79e to get more accurate results

@@           Coverage Diff           @@
##            main   #3310     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        327     329      +2     
  Lines      31840   31912     +72     
=======================================
+ Hits       31715   31789     +74     
+ Misses       125     123      -2

Impacted Files	Coverage Δ
evalml/pipelines/__init__.py	`100.0% <ø> (ø)`
evalml/pipelines/components/__init__.py	`100.0% <ø> (ø)`
...alml/pipelines/components/transformers/__init__.py	`100.0% <ø> (ø)`
evalml/tests/component_tests/test_components.py	`99.3% <ø> (ø)`
...s/prediction_explanations_tests/test_explainers.py	`100.0% <ø> (ø)`
.../components/transformers/preprocessing/__init__.py	`100.0% <100.0%> (ø)`
...formers/preprocessing/drop_nan_rows_transformer.py	`100.0% <100.0%> (ø)`
evalml/pipelines/components/utils.py	`95.3% <100.0%> (+0.2%)`	⬆️
evalml/pipelines/time_series_pipeline_base.py	`100.0% <100.0%> (ø)`
evalml/pipelines/utils.py	`99.5% <100.0%> (+0.1%)`	⬆️
... and 6 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 2398f09...826c79e. Read the comment docs.

freddyaboulton · 2022-02-11T18:14:59Z

evalml/automl/automl_algorithm/default_algorithm.py

+                and pipeline.component_graph.compute_order[-2] == name
+            ):
+                n_rows_to_drop = (
+                    self._pipeline_params["pipeline"]["max_delay"]


@christopherbunn not all of the pipelines have the same number of NaN rows. After the first batch, the number of delays, rolling window size will change as the search progresses.

from evalml.automl import AutoMLSearch from evalml.demos import load_weather X, y = load_weather() automl = AutoMLSearch(X, y, "time series regression", max_batches=4, _automl_algorithm="iterative", problem_configuration={"max_delay": 30, "forecast_horizon": 7, "gap": 1, "time_index": "Date"}, verbose=True) automl.search() light_gbm_pipelines = [automl.get_pipeline(i) for i in automl.full_rankings.loc[automl.full_rankings.pipeline_name.str.contains("LightGBM")].id] [pl.parameters['Time Series Featurizer']['conf_level'] for pl in light_gbm_pipelines] [pl.parameters['Time Series Featurizer']['rolling_window_size'] for pl in light_gbm_pipelines] fitted_light_gbm_pipelines = [pl.fit(X, y) for pl in light_gbm_pipelines] [pl.transform_all_but_final(X, y).isna().sum(axis=0).max() for pl in fitted_light_gbm_pipelines]

Note that the number of NaN rows is never max_delay + forecast_horizon + gap. I worry we may be throwing out too much data with the approach. I like the idea of only dropping NaNs for some estimators. Can we test with DropNaN component for the estimators that need it for more than two batches?

Sure! Here's the results I got for first N vs. drop NaN only for select estimators. It looks like across the board we're getting better results with the drop NaN component than the drop rows component. As you pointed out, it's most likely because we're dropping rows that do not have NaN values. I'll adjust the preprocessing chain to use the drop NaN component for time series pipelines when needed.

drop_n_vs_drop_nan_necessary.html.zip

freddyaboulton

@christopherbunn Looks good to me. I think we just need to make sure the DropNaNRows... doesn't reset the woodwork schema. Let's add a test for that too.

freddyaboulton · 2022-02-14T21:27:17Z

evalml/pipelines/components/transformers/preprocessing/drop_nan_rows_transformer.py

+        y_t = infer_feature_types(y) if y is not None else None
+
+        X_t, y_t = drop_rows_with_nans(X_t, y_t)
+        X_t.ww.init()


@christopherbunn I think we need to store the schema prior to drop and then call init with the old schema so that we make sure we don't reset the schema.

christopherbunn

@freddyaboulton do you mind taking another look? the DefaultAlgorithm as the default got merged to main after your review and I ended up having to make some changes...

evalml/pipelines/components/transformers/preprocessing/drop_nan_rows_transformer.py

christopherbunn · 2022-02-15T14:50:53Z

evalml/pipelines/utils.py

+        if need_drop_nan:
+            last_component_name = pipeline.component_graph.get_last_component().name
+            pipeline.component_graph.component_dict[estimator.name] = [
+                estimator,
+                last_component_name + ".x",
+                last_component_name + ".y",
+            ]


I feel like this and lines 381-383 + 392 are a bit hacky, but I couldn't find a better function to do this.

OK, yea, this part seems a little hacky. To make sure I understand, we're modifying the component_graph so that the estimator is fed by the DropNanRowsTransfomer, which can modify the target. But why can't we do this within _make_pipeline_from_multiple_graphs()? Wouldn't that be the function responsible for that logic?

Yep, my current implementation basically has both sub pipelines feed into the DropNaNRowsTransformer. The drop NaN rows transformer then feeds that output into the estimator.

I know I asked for a better alternative to this implementation, but I'm a bit hesitant to move this logic into _make_pipeline_from_multiple_graphs. I think that this specific function should just be responsible for combining graphs. Having the insert DropNaNRowsTransformer logic could make maintaining _make_pipeline_from_multiple_graphs() confusing in the long run.

I guess my ideal would be the ability for _make_pipelines_from_multiple_graphs() to take in a "postprocessing graph" of multiple components rather than just a single estimator, but I think adding that functionality should be its own issue.

I think this is another example of why we need this: #2997

christopherbunn · 2022-02-15T14:51:56Z

evalml/tests/component_tests/test_drop_nan_rows_transformer.py

+def test_drop_rows_transformer_retain_ww_schema():
+    # Expecting float because of np.NaN values
+    X = pd.DataFrame(
+        {"a column": [np.NaN, 2, 3, 4], "another col": ["a", np.NaN, "c", "d"]}
+    )
+    X.ww.init()
+    X.ww.set_types(
+        logical_types={"another col": "PersonFullName"},
+        semantic_tags={"a column": "custom_tag"},
+    )
+
+    X_expected = pd.DataFrame({"a column": [3], "another col": ["c"]}, index=[2])
+    X_expected = X_expected.astype({"a column": "float", "another col": "string"})
+    X_expected_schema = X.ww.schema
+
+    y = pd.Series([3, 2, 1, np.NaN])
+    y = init_series(y, logical_type="IntegerNullable", semantic_tags="y_custom_tag")
+
+    y_expected = pd.Series([True], index=[2])
+    y_expected = init_series(
+        y_expected, logical_type="IntegerNullable", semantic_tags="y_custom_tag"
+    )
+    y_expected_schema = y.ww.schema
+
+    drop_rows_transformer = DropNaNRowsTransformer()
+    transformed_X, transformed_y = drop_rows_transformer.fit_transform(X, y)
+    assert_frame_equal(transformed_X, X_expected)
+    assert_series_equal(transformed_y, y_expected)
+    assert _schema_is_equal(transformed_X.ww.schema, X_expected_schema)
+    assert transformed_y.ww.schema == y_expected_schema


Saw I forgot to include this file in the last push 😅 but added a ww check test

chukarsten

Hey Chris, great work, as per usual. I think we should reconsider refactoring out the test for what needs the DropNanRowsTransformer so that it can be reused in the places I call out in the review. That will protect us from any weird divergence bugs where maybe we don't update both. Also had a lingering question about why we can't let the normal make_pipeline style functions handle the component graph adjustments!

evalml/pipelines/components/transformers/preprocessing/drop_nan_rows_transformer.py

evalml/pipelines/utils.py

chukarsten · 2022-02-15T16:34:37Z

evalml/pipelines/utils.py

+        if need_drop_nan:
+            last_component_name = pipeline.component_graph.get_last_component().name
+            pipeline.component_graph.component_dict[estimator.name] = [
+                estimator,
+                last_component_name + ".x",
+                last_component_name + ".y",
+            ]


OK, yea, this part seems a little hacky. To make sure I understand, we're modifying the component_graph so that the estimator is fed by the DropNanRowsTransfomer, which can modify the target. But why can't we do this within _make_pipeline_from_multiple_graphs()? Wouldn't that be the function responsible for that logic?

evalml/tests/component_tests/test_drop_nan_rows_transformer.py

evalml/tests/pipeline_tests/test_pipeline_utils.py

freddyaboulton

Thank you @christopherbunn !

freddyaboulton · 2022-02-16T18:33:01Z

evalml/pipelines/components/utils.py

    return estimator_classes


+def estimator_unable_to_handle_nans(estimator_class):


It may be safer to implement this as the opposite, e.g. "able_to_handle_nans". The concern I have with doing it like this is that if we add a new estimator, this function will return False if we forget to update this file and a false negative in this case is costly because it will mean AutoMLSearch will error out.

Not blocking - let's handle it in a follow-up.

chukarsten

Looks good, thanks for making the changes and making them so quickly!

christopherbunn force-pushed the 3259_drop_nan_rows_component branch from f9934d6 to 8b0365c Compare February 8, 2022 17:00

christopherbunn changed the title ~~Add drop first N component and removed drop NaN from time series fit~~ Replaced drop NaN for time series with drop rows component Feb 8, 2022

christopherbunn force-pushed the 3259_drop_nan_rows_component branch 2 times, most recently from 4b597b9 to 724b53c Compare February 10, 2022 19:54

christopherbunn requested a review from a team February 10, 2022 22:23

christopherbunn marked this pull request as ready for review February 11, 2022 16:10

auto-assign bot assigned christopherbunn Feb 11, 2022

freddyaboulton reviewed Feb 11, 2022

View reviewed changes

christopherbunn force-pushed the 3259_drop_nan_rows_component branch from 03b53ff to 5384c6d Compare February 14, 2022 20:14

christopherbunn changed the title ~~Replaced drop NaN for time series with drop rows component~~ Added drop NaN component to some time series pipelines Feb 14, 2022

christopherbunn force-pushed the 3259_drop_nan_rows_component branch from a3e9f65 to 558dca1 Compare February 14, 2022 20:48

freddyaboulton approved these changes Feb 14, 2022

View reviewed changes

christopherbunn force-pushed the 3259_drop_nan_rows_component branch from 37c1f64 to 659738d Compare February 15, 2022 05:20

christopherbunn commented Feb 15, 2022

View reviewed changes

christopherbunn force-pushed the 3259_drop_nan_rows_component branch from 659738d to 1ac479e Compare February 15, 2022 14:53

chukarsten suggested changes Feb 15, 2022

View reviewed changes

christopherbunn requested a review from chukarsten February 16, 2022 17:19

freddyaboulton approved these changes Feb 16, 2022

View reviewed changes

chukarsten mentioned this pull request Feb 17, 2022

Create WW retain schema integration test for all components #3338

Open

chukarsten approved these changes Feb 17, 2022

View reviewed changes

christopherbunn added 9 commits February 17, 2022 12:47

First commit

65cfcac

Moved to drop nan rows for some

2404251

Drop all nan

4b525d9

Moved to drop n rows/necessary estimators

68f279a

Removed transformer and updated release notes

06cc036

Remove extra drop nan

72e7438

Fixed explainers test

b025256

Updated tests

d4768fd

Fix final test

773228c

christopherbunn added 14 commits February 17, 2022 12:47

Updated drop rows transformer tests

fb33d9b

Update test

ba1eef9

Lint and test fixes

564f2e0

Lint fix

cbcf807

Use drop NaN rows on some instead

69d8457

Updated tests for NaN component

7da530b

Updated release notes

f6b56a7

Lint fix

5f2bceb

Updated regressions

641cf15

Added woodwork retention and tests

7af50dc

Updated time series pipeline building

4927958

lint fix

446bb59

Separated out estimator needs nan function + refactors

7060238

Fixed estimator nan check function

826c79e

christopherbunn force-pushed the 3259_drop_nan_rows_component branch from 395383c to 826c79e Compare February 17, 2022 17:47

christopherbunn merged commit c7d229a into main Feb 17, 2022

chukarsten mentioned this pull request Feb 18, 2022

Release v0.45.0 #3344

Merged

		return estimator_classes


		def estimator_unable_to_handle_nans(estimator_class):

Added drop NaN component to some time series pipelines #3310

Added drop NaN component to some time series pipelines #3310

Uh oh!

Conversation

christopherbunn commented Feb 8, 2022

Uh oh!

codecov bot commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freddyaboulton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherbunn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherbunn Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chukarsten left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

freddyaboulton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chukarsten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 8, 2022 •

edited

Loading

christopherbunn Feb 15, 2022 •

edited

Loading