Add nullable type incompatibility properties to the components that have them #4031

tamargrey · 2023-02-24T18:31:25Z

Adds accurate _integer_nullable_incompatibilities and _boolean_nullable_incompatibilities properties to the components that are known to have nullable type incompatibilities. Tests that those incompatibilities cause errors that are fixed by the handling. Also tests that components without incompatibilities can use the nullable types.

Note - does not actually start calling _handle_nullable_types inside fit/predict/transform yet, as that would start using it in automl search. So we do not yet expect to see any impact to search performance or runtimes.

codecov · 2023-02-24T18:39:14Z

Codecov Report

Merging #4031 (1ceef7f) into main (337b44f) will increase coverage by 0.1%.
The diff coverage is 100.0%.

❗ Current head 1ceef7f differs from pull request most recent head 36723c8. Consider uploading reports for the commit 36723c8 to get more accurate results

@@           Coverage Diff           @@
##            main   #4031     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        349     349             
  Lines      37257   37472    +215     
=======================================
+ Hits       37136   37354    +218     
+ Misses       121     118      -3

Impacted Files	Coverage Δ
...ents/estimators/classifiers/lightgbm_classifier.py	`100.0% <100.0%> (ø)`
...omponents/estimators/regressors/arima_regressor.py	`100.0% <100.0%> (ø)`
...tors/regressors/exponential_smoothing_regressor.py	`100.0% <100.0%> (ø)`
...onents/estimators/regressors/lightgbm_regressor.py	`100.0% <100.0%> (ø)`
...nents/transformers/imputers/time_series_imputer.py	`100.0% <100.0%> (ø)`
...es/components/transformers/samplers/oversampler.py	`100.0% <100.0%> (ø)`
...alml/tests/component_tests/test_arima_regressor.py	`100.0% <100.0%> (ø)`
evalml/tests/component_tests/test_components.py	`99.1% <100.0%> (+0.1%)`	⬆️
...nent_tests/test_exponential_smoothing_regressor.py	`100.0% <100.0%> (ø)`
...alml/tests/component_tests/test_lgbm_classifier.py	`100.0% <100.0%> (ø)`
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

tamargrey · 2023-02-24T18:42:42Z

evalml/pipelines/components/transformers/imputers/time_series_imputer.py

            subset_cols=X_schema._filter_cols(
-                exclude=["IntegerNullable", "BooleanNullable"],
+                exclude=["IntegerNullable", "BooleanNullable", "AgeNullable"],


We'll likely remove this logic once we integrate the new nullable type handling into AutoMLSearch, but I wanted to add in AgeNullable here so that we could test with it now.

The one reason we may keep is is that nans are present at this stage, so we'll have to convert types to Double and Categorical, and we may want logic to not keep those types once nans are gone and we can use Integer and Boolean. I'll put more thought into this when I get to #3999

tamargrey · 2023-02-24T18:46:48Z

evalml/tests/component_tests/test_arima_regressor.py

+    evalml_arima.fit(X_train, y_train)
+
+    # Confirm that the handle nullable types method fixes the error for AutoARIMA
+    X_train_d, y_train_d = evalml_arima._handle_nullable_types(X_train, y_train)


Once we integrate the new handling into automl search, this call to _handle_nullable_types will be inside fit and predict, so we'll remove this. Keeping the error checking for sk_arima will help us know when sktime adds support for integer nullable on arima.

tamargrey · 2023-02-24T18:48:06Z

evalml/tests/component_tests/test_components.py

+    ["BooleanNullable", "IntegerNullable", "AgeNullable"],
+)
+# --> this is 180 tests - is it overkill?
+def test_components_support_nullable_types(


I feel like we want the tests for making sure our compatible components are actually compatible to be exhaustive. This does add ~180 tests, though, so I just want to make sure there's visibility around that.

tamargrey · 2023-02-24T18:49:22Z

evalml/tests/component_tests/test_estimators.py

+
+        with pytest.raises(Exception):
+            comp.fit(X, y)
+            comp.predict(X)


Because we are using the actaul evalml components here, this will have to change once we integrate into automl search to call the underlying component_obj so that we can track when support gets added for that component.

tamargrey · 2023-02-24T18:50:19Z

evalml/tests/component_tests/test_oversampler.py

+                "Unknown label type: 'unknown'",
+            ),
+        ):
+            oversampler.transform(X, y)


similar to the test_estimators.py test - we are using the actual evalml component here, so this will have to change once we integrate into automl search

tamargrey · 2023-02-24T18:51:01Z

evalml/tests/conftest.py

        if has_nans:
            y = pd.Series([1, 0, pd.NA, 1, 0] * 4)
        else:
-            y = pd.Series([1, 0, 1, 1, 0] * 4)
+            y = pd.Series([1, 0, 1, 1, 1] * 4)


Changed so I could test the oversampler better :)

jeremyliweishih

LGTM - just one clarifying question

jeremyliweishih · 2023-02-24T19:17:20Z

evalml/tests/component_tests/test_components.py

+        X.ww.set_types(logical_types={"feature": "IntegerNullable"})
+        X.ww["bool col"] = bool_col
+    else:
+        y = nullable_type_target(ltype=nullable_y_ltype, has_nans=False)


why don't we run y = nullable_type_target(ltype=nullable_y_ltype, has_nans=False) for components that require the time index?

I probably could; I'd just need to change nullable_type_target to be the correct length for ts_data and also update it to have the same datetime index values as X. I felt like it was simpler to just create bool_col and set y to be it.

eccabay

Looks pretty good! Mostly just test cleanups and clarifying questions, but nothing blocking.

eccabay · 2023-02-27T15:11:48Z

evalml/pipelines/components/transformers/imputers/time_series_imputer.py

@@ -194,3 +199,21 @@ def transform(self, X, y=None):
            y_imputed = ww.init_series(y_imputed)

        return X_not_all_null, y_imputed
+
+    def _handle_nullable_types(self, X=None, y=None):
+        """Transforms X and y to remove any incompatible nullable types for the time series imputer when the interpolate method is used.


Why do we need to treat interpolation differently than the other fill methods? And will this remove any necessary handling for the other methods?

interpolation is the only one that has any incompatibilities with nullable types, so there's no need to remove nullable types for the other fill methods.

evalml/pipelines/components/transformers/imputers/time_series_imputer.py

evalml/tests/component_tests/test_components.py

evalml/tests/component_tests/test_estimators.py

evalml/tests/component_tests/test_lgbm_classifier.py

evalml/tests/component_tests/test_lgbm_regressor.py

evalml/tests/component_tests/test_time_series_imputer.py

eccabay · 2023-02-27T20:17:09Z

evalml/tests/component_tests/test_time_series_imputer.py

@@ -558,3 +560,101 @@ def test_imputer_woodwork_custom_overrides_returned_by_components(
        transformed.ww.logical_types["categorical with nan"]
        == X.ww.logical_types["categorical with nan"]
    )
+
+
+def test_imputer_nullable_handling_numeric_interpolate(nullable_type_test_data):


I'm not convinced that these tests are how we want to be testing the TimeSeriesImputer._handle_nullable_types. It feels like too much is testing sklearn's interpolate, rather than our imputation. I do see why you did it, since just doing the (running the component fails -> calling _handle_nullable_types fixes it) check of other estimators doesn't work here. I think it would be better to combine this test and the one below into one, and focus on testing that the logical splits to only handle nullable types in the interpolation case works as intended. We already have tests ensuring that the super()._handle_nullable_types has the desired behavior.

This is a fair point, and I recognize that we do not generally want to be testing our dependencies' behaviors. Part of the goal of this, beyond confirming that our handling lets us avoid the problem, is that this check will notify us when support is added. I have a similar check in the arima tests, and am planning to eventually have them for all incompatibilities.

This is just my preference for how to keep track of our dependencies adding support, though. I recognize it means that when a pandas version is released with support interpolating nullable types, for example, we'll get an error that we will need to handle before we can merge any dependency updates.

That will be a bit of a pain, but it allows us to remove this handling when possible without having to manually keep track of when dependencies add support, which I would argue is more of a pain. Happy to open this up to a larger discussion with the rest of the team.

tamargrey · 2023-03-01T14:52:08Z

evalml/tests/component_tests/test_time_series_imputer.py

+    ["BooleanNullable", "IntegerNullable", "AgeNullable"],
+)
+@pytest.mark.xfail(strict=True, raises=ValueError)
+def test_time_series_imputer_nullable_type_incompatibility(


@chukarsten @eccabay @jeremyliweishih I'm still adding the rest of the xfail tests but wanted to run this by yall.

I have the xfail set to only expect ValueError, but unfortunately, we can't specify the expected message. Would people prefer I get as specific about what the expected message is? My thought here is just that I feel like being able to use this format of xfailing where we write code that errors and say that the code is expected to error feels more true to our goal here than triggering the xfail from the test directly when catch the error ourselves.

The strict parameter means that our test suite will fail when tests start passing - otherwise they just show as XPASS which I think would be easy to miss. But if we want to avoid blocking the update checker, we could set this to False and just periodically remember to check. I think that's probably a halfway point between what I'm suggesting and the fully manual checking process.

This method of xfail seems like the best to me!

I'm a fan of keeping strict=True. It's really easy to miss an XPASS, and we don't have any way of flagging them at this point. I'm pretty sure we already have a bunch of XPASSing tests that we might want to revisit...

sliiight change so that we can test that our _handle_nullable_types fixes the incompatibility - we will expect tests to pass when we handle incompatibilities and xfail other times

@pytest.mark.parametrize( "nullable_ltype", ["BooleanNullable", "IntegerNullable", "AgeNullable"], ) @pytest.mark.parametrize( "handle_incompatibility", [ True, pytest.param( False, marks=pytest.mark.xfail(strict=True, raises=ValueError), ), ], ) def test_time_series_imputer_nullable_type_incompatibility( nullable_type_target, handle_incompatibility, nullable_ltype, ): """Testing that the nullable type incompatibility that caused us to add handling for the time series imputer is still present in pandas' interpolate method. If this test is causing the test suite to fail because the code below no longer raises the expected ValueError, we should confirm that the nullable types now work for our use case and remove the nullable type handling logic from TimeSeriesImputer.""" nullable_series = nullable_type_target(ltype=nullable_ltype, has_nans=True) if handle_incompatibility: imputer = TimeSeriesImputer(target_impute_strategy="interpolate") imputer.fit(pd.DataFrame(), nullable_series) _, nullable_series = imputer._handle_nullable_types(None, nullable_series) nullable_series.interpolate()

tamargrey · 2023-03-01T18:32:22Z

evalml/pipelines/components/transformers/imputers/time_series_imputer.py

+            # since the category dtype also has incompatibilities with linear interpolate, which is expected
+            # --> i think this is essentially what happens now but won't that make floating point values that cant be turned back into bools?
+            if isinstance(y.ww.logical_type, BooleanNullable):
+                y = ww.init_series(y, Double)


Noticed that interpolate() cannot handle category dtypes either. But this, I think, is by design, as the default interpolation is "linear" which doesnt make sense for non numeric values. This is problematic if we try to turn BooleanNullable into Categorical ltype when nans are present, so I'm intercepting it and converting to Double, which essentially the current behavior.

In the long run, though, I think we don't want to be doing linear interpolation on boolean columns ever - we don't want to end up with 1.5 in a column of all 1s and 0s. I'll open a separate evalml ticket for handling that, though.

makes sense!

We already don't allow using interpolation with categorical or boolean types. From the docstring:

categorical_impute_strategy (string): Impute strategy to use for string, object, boolean, categorical dtypes. Valid values include "backwards_fill" and "forwards_fill". Defaults to "forwards_fill". numeric_impute_strategy (string): Impute strategy to use for numeric columns. Valid values include "backwards_fill", "forwards_fill", and "interpolate". Defaults to "interpolate".

It's enforced in a couple different places, between checking the arguments and how we select splitting between numeric and categorical!

I only ran into it with the target impute strategy, which I think doesn't have the same safeguards.

Ah, very true - good point!

jeremyliweishih

LGTM - great work!

jeremyliweishih · 2023-03-01T19:20:04Z

evalml/pipelines/components/transformers/imputers/time_series_imputer.py

+            # since the category dtype also has incompatibilities with linear interpolate, which is expected
+            # --> i think this is essentially what happens now but won't that make floating point values that cant be turned back into bools?
+            if isinstance(y.ww.logical_type, BooleanNullable):
+                y = ww.init_series(y, Double)


makes sense!

…nt really work

…pe incompatibiliy

tamargrey commented Feb 24, 2023

View reviewed changes

tamargrey force-pushed the component-nullable-handling branch from 5580611 to 9735b95 Compare February 24, 2023 18:57

tamargrey marked this pull request as ready for review February 24, 2023 18:57

auto-assign bot assigned tamargrey Feb 24, 2023

tamargrey requested review from jeremyliweishih, chukarsten, eccabay and ParthivNaresh February 24, 2023 19:04

jeremyliweishih approved these changes Feb 24, 2023

View reviewed changes

tamargrey force-pushed the component-nullable-handling branch from 9735b95 to 9e72874 Compare February 27, 2023 19:05

eccabay approved these changes Feb 27, 2023

View reviewed changes

tamargrey commented Mar 1, 2023

View reviewed changes

tamargrey requested review from eccabay and jeremyliweishih March 1, 2023 18:33

tamargrey force-pushed the component-nullable-handling branch from 3f84baa to 368b5a3 Compare March 1, 2023 18:34

jeremyliweishih approved these changes Mar 1, 2023

View reviewed changes

tamargrey force-pushed the component-nullable-handling branch 3 times, most recently from c159a46 to 1ceef7f Compare March 2, 2023 14:45

Tamar Grey added 4 commits March 2, 2023 11:00

Add incompatibilities to components and make catch all test that does…

93de7cb

…nt really work

Add arima test for handling

7613d57

Add special handle method for ts imputer and tests

38e1eae

Add oversampler test for handling

9b8900b

Tamar Grey added 12 commits March 2, 2023 11:00

Add estimator test for remaining incompatible estimators

f19742e

Add tests for components that can handle nullable types

6f8a170

Add release note

c96b717

Clean up

f202e53

Add lightgbm tests

3eb0b6b

Add exponential smoothing test and test compatible nullable types

0c4fdc2

PR Comments

80f7eca

Add xfail for incompatibility in interpolate

6ba78cf

restructure time series imputer tests with xfail

6cfbc1a

Restructure remaining components' tests to have xfail for nullable ty…

25f619f

…pe incompatibiliy

Remove comment

fc58d3b

lint fix

36723c8

tamargrey force-pushed the component-nullable-handling branch from 1ceef7f to 36723c8 Compare March 2, 2023 16:00

tamargrey enabled auto-merge (squash) March 2, 2023 16:21

tamargrey merged commit 33eb2c6 into main Mar 2, 2023

tamargrey deleted the component-nullable-handling branch March 2, 2023 16:31

chukarsten mentioned this pull request Mar 15, 2023

Release v0.69.0 #4078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nullable type incompatibility properties to the components that have them #4031

Add nullable type incompatibility properties to the components that have them #4031

tamargrey commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

tamargrey Feb 24, 2023

tamargrey Feb 24, 2023

tamargrey Feb 24, 2023 •

edited

Loading

tamargrey Feb 24, 2023

tamargrey Feb 24, 2023

tamargrey Feb 24, 2023

jeremyliweishih left a comment

jeremyliweishih Feb 24, 2023

tamargrey Feb 24, 2023

eccabay left a comment

eccabay Feb 27, 2023

tamargrey Feb 27, 2023

eccabay Feb 27, 2023

tamargrey Feb 27, 2023

tamargrey Mar 1, 2023 •

edited

Loading

eccabay Mar 1, 2023

tamargrey Mar 1, 2023

tamargrey Mar 1, 2023

jeremyliweishih Mar 1, 2023

eccabay Mar 1, 2023

tamargrey Mar 1, 2023

eccabay Mar 1, 2023

jeremyliweishih left a comment

jeremyliweishih Mar 1, 2023

Add nullable type incompatibility properties to the components that have them #4031

Add nullable type incompatibility properties to the components that have them #4031

Conversation

tamargrey commented Feb 24, 2023 • edited Loading

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamargrey Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyliweishih left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eccabay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamargrey Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyliweishih left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamargrey commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

tamargrey Feb 24, 2023 •

edited

Loading

tamargrey Mar 1, 2023 •

edited

Loading