Pandas 1.4.0 Support by chukarsten · Pull Request #3324 · alteryx/evalml

chukarsten · 2022-02-14T05:38:10Z

Like the issue says, there are basically three areas to address:

XGBoost future warning: This is handled by expecting this test to fail on this PR and then, when XGBoost does their next release, fixing the auto dependency update as the test will no longer fail as there will be no more warning.
Results of email featurization: The underlying problem is that featuretools is returning a CategoricalDtype with the dtype of "string" when we, in our test, build an expected dataframe with CategoricalDtype with dtype of "object." I'm not entirely sure why this is the case as I've stepped through both the woodwork accessor and featuretools trying to find out how this happens. I've also tried to reproduce making a CategoricalDtype with categories that have dtype "string" but to no avail. I've rewritten the equality check here to check the values, column names, indices and logical types separately as a stop-gap until this behavior is understood.
Imputer/SimpleImputer failures: We decided that the NaturalLanguage should not be imputed, so it is imputed, no longer.

codecov · 2022-02-14T05:43:10Z

Codecov Report

Merging #3324 (8779828) into main (6220044) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3324     +/-   ##
=======================================
- Coverage   99.7%   99.6%   -0.0%     
=======================================
  Files        329     329             
  Lines      31912   31977     +65     
=======================================
+ Hits       31788   31847     +59     
- Misses       124     130      +6

Impacted Files	Coverage Δ
...components/transformers/imputers/simple_imputer.py	`100.0% <100.0%> (ø)`
...nt_tests/test_ft_transform_primitive_components.py	`100.0% <100.0%> (ø)`
evalml/tests/component_tests/test_imputer.py	`100.0% <100.0%> (ø)`
...valml/tests/component_tests/test_simple_imputer.py	`100.0% <100.0%> (ø)`
...l/tests/component_tests/test_xgboost_classifier.py	`93.1% <100.0%> (-6.9%)`	⬇️
evalml/tests/conftest.py	`97.1% <100.0%> (+0.1%)`	⬆️

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 6220044...8779828. Read the comment docs.

chukarsten · 2022-02-15T16:05:47Z

evalml/tests/component_tests/test_xgboost_classifier.py

    assert not preds.isna().any()


+@pytest.mark.xfail


Marking this xfail so that we can explicitly remove it when XGBoost releases the fixing change! The related issue is #3331 so we don't forget.

chukarsten · 2022-02-15T16:08:41Z

evalml/tests/component_tests/test_simple_imputer.py

-            }
+
+    # Categorical -> Boolean fails in infer_feature_types
+    if data == "categorical_data" and logical_type == Boolean:


Simplified some of the logic here. The only failure that seems to occur between the override types and the dataframe that's going in is going from categorical data to Boolean. This seems to trigger a Woodwork conversion failure.

chukarsten · 2022-02-15T20:20:35Z

evalml/tests/component_tests/test_simple_imputer.py

-def test_simple_imputer_supports_natural_language_constant():
+@pytest.mark.parametrize("na_type", ["python_none", "numpy_nan", "pandas_na"])
+@pytest.mark.parametrize("data_type", ["Categorical", "NaturalLanguage"])
+def test_simple_imputer_supports_natural_language_and_categorical_constant(


Since these tests failed, I just refactored them to be more descriptive per the parameters in case they should fail again.

chukarsten · 2022-02-15T20:21:21Z

evalml/tests/component_tests/test_simple_imputer.py

-                ["free-form text", "will", "be imputed", "placeholder"], dtype="string"
+            "Categorical": pd.Series(["a", "b", "a", "placeholder"], dtype="category"),
+            "NaturalLanguage": pd.Series(
+                ["free-form text", "will", "be imputed", pd.NA], dtype="string"


Might be a little redundant, but the imputer now ignores the natural language columns.

I think it only ignores natural language columns if the only column in the dataframe is a natural language column?

from evalml.pipelines.components import SimpleImputer import pandas as pd import woodwork as ww import pytest X = pd.DataFrame({"a": ["foo", "bar", "baz"], "b": [1, 2, 3]}) X.ww.init(logical_types={"a": "NaturalLanguage"}) si = SimpleImputer() with pytest.raises(ValueError): si.fit_transform(X)

I think it might be better to raise an error if non-imputable types are present?

@freddyaboulton I'm more of a fan of ignoring the columns if our imputer explicitly (we should not this in the docstring if we pursue this path) does not support natural language columns and that's the design decision I made with all of the select column transformers. If we raise an error it would mean that we would error out during search if the input contains a natural language column with nulls (but I'm unsure how the NaturalLanguageFeaturizer deals with nulls). Maybe raising a warning is a happy medium?

@chukarsten if we want to keep with the ignoring path - we would need to add the natural language columns back in after transform-ing as well.

chukarsten · 2022-02-15T20:22:29Z

evalml/tests/component_tests/test_simple_imputer.py

+    }[logical_type]
    y = pd.Series([1, 2, 1])
-    override_types = [Integer, Double, Categorical, NaturalLanguage, Boolean]
-    for logical_type in override_types:


Refactored this test so that all the parameters are listed in the test failure sections and it's easier to understand what's failing, specifically which override type.

freddyaboulton

@chukarsten Looks good to me! I just want to synch up on the expected behavior of natural language in the simple imputer prior to merge.

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

freddyaboulton · 2022-02-15T21:43:50Z

evalml/tests/component_tests/test_simple_imputer.py

-                ["free-form text", "will", "be imputed", "placeholder"], dtype="string"
+            "Categorical": pd.Series(["a", "b", "a", "placeholder"], dtype="category"),
+            "NaturalLanguage": pd.Series(
+                ["free-form text", "will", "be imputed", pd.NA], dtype="string"


I think it only ignores natural language columns if the only column in the dataframe is a natural language column?

from evalml.pipelines.components import SimpleImputer import pandas as pd import woodwork as ww import pytest X = pd.DataFrame({"a": ["foo", "bar", "baz"], "b": [1, 2, 3]}) X.ww.init(logical_types={"a": "NaturalLanguage"}) si = SimpleImputer() with pytest.raises(ValueError): si.fit_transform(X)

I think it might be better to raise an error if non-imputable types are present?

evalml/tests/component_tests/test_imputer.py

freddyaboulton · 2022-02-15T21:54:09Z

evalml/tests/component_tests/test_ft_transform_primitive_components.py

    new_X = component.transform(data)

-    pd.testing.assert_frame_equal(new_X, expected)
+    # TODO: Understand why the "EMAIL_ADDRESS_TO_DOMAIN(email)" engineered feature


I think the new change in behavior with pandas 1.4.0 is that the categories attribute of the categorical dtype now includes "original" feature values. Since the physical type of URL/EmailAddress logical types is "string" (nullable), that's why the categories have nullable values.

I think we can get around this behavior if we update the make_answer functions but I think what we have here is fine too.

Do you know how to do that? I pinged the woodwork team for how we can add in that specific column with the "string" type, but I can't seem to do it.

I think like this:

Not blocking though. The current equality logic in the test is sufficient.

jeremyliweishih · 2022-02-15T21:44:11Z

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

+        # Not using select because we just need column names, not a new dataframe
+        natural_language_columns = self._get_columns_of_type(X, NaturalLanguage)
+        if natural_language_columns:
+            X = X.ww.copy()


why do we need to copy here and in _set_boolean_columns_to_categorical?

docs/source/release_notes.rst

evalml/tests/component_tests/test_ft_transform_primitive_components.py

evalml/tests/component_tests/test_imputer.py

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

jeremyliweishih · 2022-02-15T22:12:49Z

evalml/tests/component_tests/test_simple_imputer.py

-                ["free-form text", "will", "be imputed", "placeholder"], dtype="string"
+            "Categorical": pd.Series(["a", "b", "a", "placeholder"], dtype="category"),
+            "NaturalLanguage": pd.Series(
+                ["free-form text", "will", "be imputed", pd.NA], dtype="string"


@freddyaboulton I'm more of a fan of ignoring the columns if our imputer explicitly (we should not this in the docstring if we pursue this path) does not support natural language columns and that's the design decision I made with all of the select column transformers. If we raise an error it would mean that we would error out during search if the input contains a natural language column with nulls (but I'm unsure how the NaturalLanguageFeaturizer deals with nulls). Maybe raising a warning is a happy medium?

@chukarsten if we want to keep with the ignoring path - we would need to add the natural language columns back in after transform-ing as well.

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

evalml/tests/component_tests/test_imputer.py

bchen1116

These changes are looking good! Agreed about moving some of the dfs to the conftest.py file, as well as left a few questions.

As per Freddy's comment here, it could be useful to add a non-categorical/NL column to the test_simple_imputer_ignores_natural_language in test_simple_imputer to ensure that this continues to work as we want.

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

evalml/tests/component_tests/test_imputer.py

evalml/tests/component_tests/test_simple_imputer.py

…data fixture. Moved the imputer_test_data fixture up into conftest.py so that test_simple_imputer could borrow it.

…ce of multiple types.

…re detected during SimpleImputer fit AND NaturalLanguage is one of them.

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

freddyaboulton

thank you @chukarsten !

chukarsten force-pushed the pandas_1.4.0_support_take_2 branch from 4975994 to 4327631 Compare February 15, 2022 15:36

chukarsten mentioned this pull request Feb 15, 2022

Revert changes to testing with new XGBoost release #3331

Open

chukarsten commented Feb 15, 2022

View reviewed changes

chukarsten marked this pull request as ready for review February 15, 2022 20:23

auto-assign bot assigned chukarsten Feb 15, 2022

chukarsten requested review from ParthivNaresh, angela97lin, bchen1116, christopherbunn, dsherry, eccabay, freddyaboulton and jeremyliweishih and removed request for angela97lin February 15, 2022 20:23

freddyaboulton reviewed Feb 15, 2022

View reviewed changes

jeremyliweishih reviewed Feb 15, 2022

View reviewed changes

angela97lin reviewed Feb 15, 2022

View reviewed changes

evalml/pipelines/components/transformers/imputers/simple_imputer.py Outdated Show resolved Hide resolved

angela97lin reviewed Feb 15, 2022

View reviewed changes

evalml/pipelines/components/transformers/imputers/simple_imputer.py Show resolved Hide resolved

angela97lin reviewed Feb 15, 2022

View reviewed changes

evalml/tests/component_tests/test_imputer.py Outdated Show resolved Hide resolved

bchen1116 approved these changes Feb 16, 2022

View reviewed changes

evalml/pipelines/components/transformers/imputers/simple_imputer.py Outdated Show resolved Hide resolved

evalml/tests/component_tests/test_imputer.py Show resolved Hide resolved

evalml/tests/component_tests/test_simple_imputer.py Outdated Show resolved Hide resolved

chukarsten force-pushed the pandas_1.4.0_support_take_2 branch from ccefefb to 416d296 Compare February 17, 2022 17:20

chukarsten added 7 commits February 17, 2022 14:01

Initial.

3d8f7b9

Got the Imputer and SimpleImputer tests working.

ba1d69f

Release notes.

7892af0

Lint

f0c267c

Lint, docstring fix and latest_release_changes.

29243fc

Removed unnecessary check and exception.

8eba539

Some coverage issues and comments.

51ec0d1

chukarsten added 12 commits February 17, 2022 14:01

Fixed the release notes...

d5b101e

Removed unused code.

b6a5cc8

Removed second arg from drop nat lang.

d80fa67

Removed new conftest fixtures and refactored to use the imputer test_…

e0fa3c2

…data fixture. Moved the imputer_test_data fixture up into conftest.py so that test_simple_imputer could borrow it.

Updated pandas 1.4.0 dep.

6998893

Updated SimpleImputer to better ignore natural language in the presen…

e4f02b2

…ce of multiple types.

Updated the imputer to throw an exception if multiple logical types a…

ef8706e

…re detected during SimpleImputer fit AND NaturalLanguage is one of them.

Just to save the exception, but deleting...

40881b4

Removed the unnecessary exception...

6b3618d

Lint.

b36424b

Rewrote the column selection to use select.

798aa23

Lint.

686c25f

chukarsten force-pushed the pandas_1.4.0_support_take_2 branch from 975b762 to 686c25f Compare February 17, 2022 19:02

Removed unnecessary code.

08cd416

freddyaboulton reviewed Feb 17, 2022

View reviewed changes

evalml/pipelines/components/transformers/imputers/simple_imputer.py Outdated Show resolved Hide resolved

Freddy's comments about reinference of NatLang.

3436932

freddyaboulton approved these changes Feb 17, 2022

View reviewed changes

chukarsten added 2 commits February 17, 2022 17:45

Merge branch 'main' into pandas_1.4.0_support_take_2

5b6d7c5

Merge branch 'main' into pandas_1.4.0_support_take_2

8779828

chukarsten merged commit cf20224 into main Feb 18, 2022

chukarsten mentioned this pull request Feb 18, 2022

Release v0.45.0 #3344

Merged

tamargrey mentioned this pull request Feb 15, 2023

Remove Nullable type logic from Imputer Components and Refactor #3999

Closed

tamargrey mentioned this pull request Mar 1, 2023

Refactor imputer components to remove unnecessary logic #4038

Merged

Conversation

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

Uh oh!

Uh oh!

codecov bot commented Feb 14, 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!

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!

jeremyliweishih 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!

Choose a reason for hiding this comment

Uh oh!

freddyaboulton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremyliweishih 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!

Uh oh!

Uh oh!

Uh oh!

bchen1116 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chukarsten commented Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading

jeremyliweishih Feb 15, 2022 •

edited

Loading

jeremyliweishih Feb 15, 2022 •

edited

Loading