Refactor and fix make_pipeline tests to assert correct order and address new Woodwork Unknown type by angela97lin · Pull Request #2572 · alteryx/evalml

angela97lin · 2021-07-29T19:41:10Z

Closes #2553.

Tests needed to be updated to handle Woodwork's defaulting to "Unknown" rather than "NaturalLanguage". This is the case when our input is pandas type, since calling infer_feature_type will convert text columns to Unknown type, which returns a different pipeline.
Pipeline tests needed to address the new LogTransformer component that could be added to pipeline
Complete refactor of if statements and tests: many of the make_pipeline tests had similar logic, but different data. Having to update these tests for the updates in make_pipeline logic meant I had to update it in many places. It may be hard to tell from the diff, but I refactored all the tests into one parameterized test and made a fixture for the data to use. Although having one large parameterized method may seem scary, it's definitely easier to add new test cases via new columns in the df. I primarily kept all of the existing tests as is with some touchups but open to changing / removing :)
Removed Time Series Baseline Estimator in parameters dict passed. Thanks to @bchen1116's great work, a warning was raised indicating that the parameters for this component weren't used since it's not in the pipeline! 😁

codecov · 2021-07-29T19:45:33Z

Codecov Report

Merging #2572 (6afabd5) into main (5410bb5) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #2572     +/-   ##
=======================================
- Coverage   99.9%   99.9%   -0.0%     
=======================================
  Files        295     295             
  Lines      27093   26811    -282     
=======================================
- Hits       27047   26765    -282     
  Misses        46      46

Impacted Files	Coverage Δ
evalml/pipelines/utils.py	`99.2% <100.0%> (+0.1%)`	⬆️
evalml/tests/pipeline_tests/test_pipeline_utils.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 5410bb5...6afabd5. Read the comment docs.

angela97lin · 2021-08-02T17:31:38Z


 @pytest.mark.parametrize("input_type", ["pd", "ww"])
 @pytest.mark.parametrize("problem_type", ProblemTypes.all_problem_types)
-def test_make_pipeline_no_datetimes(input_type, problem_type):


I didn't see value in this compared to test_make_pipeline_text_columns so deleting :)

angela97lin · 2021-08-02T18:14:42Z

    parameters=None,
    sampler_name=None,
-    extra_components=[],
+    extra_components=None,


I think it'd be a good idea to change this default value to None, rather than an empty list. Right now, we don't modify this parameter but if we did, it could lead to unexpected behavior:

From https://docs.python.org/3/reference/compound_stmts.html#function-definitions:

Default parameter values are evaluated from left to right when the function definition is executed. This means that the expression is evaluated once, when the function is defined, and that the same “pre-computed” value is used for each call. This is especially important to understand when a default parameter is a mutable object, such as a list or a dictionary: if the function modifies the object (e.g. by appending an item to a list), the default value is in effect modified. This is generally not what was intended. A way around this is to use None as the default, and explicitly test for it in the body of the function...

Absolutely! Thankfully this didn't cause any bugs since it's not used

angela97lin · 2021-08-02T18:16:06Z

-            if is_time_series(problem_type):
-                delayed_features = [DelayedFeatureTransformer]
-            if estimator_class.model_family == ModelFamily.LINEAR_MODEL:
-                estimator_components = [StandardScaler, estimator_class]
-            elif estimator_class.model_family == ModelFamily.CATBOOST:
-                estimator_components = [estimator_class]
-            else:


I found myself being confused by the different if/else statements (some of which are related, others which are not!) so I thought this would be a clearer breakdown but let me know if you have thoughts/opinions!

Hot take: We shouldn't support plain numpy arrays anymore.

angela97lin · 2021-08-02T23:25:58Z

-            if estimator_class.model_family == ModelFamily.ARIMA:
-                expected_components = [Imputer] + estimator_components
+@pytest.fixture
+def get_test_data_from_configuration():


Fixture that contains the data for the make_pipeline test!

Love this, anything that consolidates fixtures is good.

This is awesome!

angela97lin · 2021-08-03T02:55:08Z

-            if is_time_series(problem_type):
-                parameters = {
-                    "pipeline": {"date_index": "soem dates", "gap": 1, "max_delay": 1},
-                    "Time Series Baseline Estimator": {


Not using Time Series Baseline Estimator so this is unnecessary!

angela97lin · 2021-08-03T02:55:57Z

+        ("email with other features", ["email", "numerical", "categorical"]),
+    ],
+)
+def test_make_pipeline_master(


test_description isn't being used by the code but since we no longer have the title of tests to tell us what is going on, I thought this would be useful to quickly understand what each test case is for.

Can we use it in the assert error message?

chukarsten

Solid, solid PR. I love the mass deletion and consolidation into the fixture. Very clean, very nice.

chukarsten · 2021-08-03T13:06:14Z

+                [OneHotEncoder]
+                if estimator_class.model_family != ModelFamily.CATBOOST
+                and (
+                    "categorical" in column_names
+                    or (
+                        any(ltype in column_names for ltype in ["url", "email"])
+                        and input_type == "ww"
+                    )


this formatting gave me a stroke!

chukarsten · 2021-08-03T16:14:04Z

-            if estimator_class.model_family == ModelFamily.ARIMA:
-                expected_components = [Imputer] + estimator_components
+@pytest.fixture
+def get_test_data_from_configuration():


Love this, anything that consolidates fixtures is good.

chukarsten · 2021-08-03T16:14:59Z

+                    "this is another string",
+                    "this is just another string",
+                    "evalml should handle string input",
+                    "cats are gr8",


you're a woman of both taste and distinction

freddyaboulton

@angela97lin Thank you for this!

freddyaboulton · 2021-08-03T16:36:25Z

    parameters=None,
    sampler_name=None,
-    extra_components=[],
+    extra_components=None,


Absolutely! Thankfully this didn't cause any bugs since it's not used

freddyaboulton · 2021-08-03T21:03:11Z

-            if estimator_class.model_family == ModelFamily.ARIMA:
-                expected_components = [Imputer] + estimator_components
+@pytest.fixture
+def get_test_data_from_configuration():


This is awesome!

freddyaboulton · 2021-08-03T21:04:02Z

+        ("email with other features", ["email", "numerical", "categorical"]),
+    ],
+)
+def test_make_pipeline_master(


Can we use it in the assert error message?

freddyaboulton · 2021-08-03T21:10:29Z

-                component.name for component in expected_components
-            ]
-
+@pytest.mark.parametrize(


This is awesome! Non-blocking food-for-thought comment hehe but did you consider including the expected answer in the parametrization rather than computing the answer within the test?

Pro: It'd be much easier to add future test cases that are independent of the other ones. I remember having to "fight" with the answer-generation-logic when I was adding test cases for URL and Email featurizers.

Con: There are a lot of test cases so it's a lot of data to specify. There will also be some duplicate answers across these tests.

I think this is great as-is but just wanted to put that out there.

Gosh, I love this idea a lot. The reason I decided to go with logic, though, is because the estimator also affects what the pipeline looks like, and we currently iterate through each of the estimators in the test. There's room to argue whether or not it's really necessary to loop through all of the estimators as we do or if we should just write out test cases for the special estimators such as CatBoost which can handle categorical cols natively and thus don't require a OneHotEncoder... but maybe this will be worth another iteration of refactoring.

I do agree though, I had to fight and wrangle the logic quite some bit to get all of these tests to pass, and if we add more logic, I'm not sure how well everything will continue to hold up 😂

freddyaboulton · 2021-08-03T21:11:29Z

-            if is_time_series(problem_type):
-                delayed_features = [DelayedFeatureTransformer]
-            if estimator_class.model_family == ModelFamily.LINEAR_MODEL:
-                estimator_components = [StandardScaler, estimator_class]
-            elif estimator_class.model_family == ModelFamily.CATBOOST:
-                estimator_components = [estimator_class]
-            else:


Hot take: We shouldn't support plain numpy arrays anymore.

ParthivNaresh

Great work, I love the parametrization of test cases to reduce code and improve readability!

ParthivNaresh · 2021-08-04T13:33:52Z

+    "test_description, column_names",
+    [
+        ("all nan is not categorical", ["all_null", "numerical"]),
+        ("all types", ["all_null", "categorical", "dates", "numerical"]),


Why doesn't all_types also include text, email, and url? They're all being tested anyways so definitely not a big deal, but should the test_description be changed?

bchen1116

Love the cleanup you did here! The new test is very through. Left a comment on some extra simplification, but also left a comment just for my understanding.

bchen1116 · 2021-08-04T15:31:49Z

-            ]
-
+@pytest.mark.parametrize(
+    "test_description, column_names",


is test_description just to allow people who look at the test to understand what each test is param iteration is testing?

Yup! Now that we can't use the test names to understand, I thought having test_description would make it easier to see at a quick glance what each test case is for 😁

…into 2553_make_pipeline_tests

fix one test, init

9d58334

angela97lin self-assigned this Jul 29, 2021

angela97lin added 4 commits August 2, 2021 02:46

fix another text test

8f1418c

lint

6e22cb7

Merge branch 'main' into 2553_make_pipeline_tests

7e52a6f

fix tests

4be7f6b

angela97lin changed the title ~~Fix make_pipeline tests to assert correct order and address new Woodwork Unknown type~~ Refactor and fix make_pipeline tests to assert correct order and address new Woodwork Unknown type Aug 2, 2021

refactor pipeline tests

ee66235

angela97lin commented Aug 2, 2021

View reviewed changes

angela97lin added 3 commits August 2, 2021 14:01

slowly cleaning up components

774e6ed

Merge branch 'main' into 2553_make_pipeline_tests

beeb8e0

change default

e47f286

angela97lin commented Aug 2, 2021

View reviewed changes

angela97lin added 3 commits August 2, 2021 15:29

push some broken intermediate work

b17073c

some major refactor

8462017

remove imports

fa7ca89

angela97lin commented Aug 2, 2021

View reviewed changes

angela97lin added 4 commits August 2, 2021 20:54

cleaned up

8a621e5

retrigger tests

e16093a

linting

796e9d0

release notes update'

a284d8b

angela97lin commented Aug 3, 2021

View reviewed changes

angela97lin marked this pull request as ready for review August 3, 2021 03:43

angela97lin requested review from bchen1116, christopherbunn, chukarsten and dsherry August 3, 2021 03:43

angela97lin requested review from ParthivNaresh, freddyaboulton and jeremyliweishih August 3, 2021 03:43

chukarsten approved these changes Aug 3, 2021

View reviewed changes

freddyaboulton approved these changes Aug 3, 2021

View reviewed changes

ParthivNaresh approved these changes Aug 4, 2021

View reviewed changes

bchen1116 approved these changes Aug 4, 2021

View reviewed changes

angela97lin added 7 commits August 6, 2021 17:02

some cleanup from comments

ea1de7a

Merge branch 'main' into 2553_make_pipeline_tests

cf401ae

rename test name'

da0a108

Merge branch '2553_make_pipeline_tests' of github.com:alteryx/evalml …

fb10809

…into 2553_make_pipeline_tests

rename col in test

5d39a25

add test description to assert error

5dc300e

oops move release notes

6afabd5

angela97lin merged commit f46b94d into main Aug 7, 2021

angela97lin deleted the 2553_make_pipeline_tests branch August 7, 2021 02:12

chukarsten mentioned this pull request Aug 12, 2021

Release v0.30.1 #2623

Closed

Conversation

angela97lin commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 29, 2021 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

chukarsten left a comment

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!

freddyaboulton left a comment

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!

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!

ParthivNaresh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

angela97lin commented Jul 29, 2021 •

edited

Loading

codecov Bot commented Jul 29, 2021 •

edited

Loading