Add SelectDtypeColumns component by eccabay · Pull Request #2531 · alteryx/evalml

eccabay · 2021-07-20T21:02:41Z

I'm totally open to changing the name of the class, it was just the first thing I thought of

CLAassistant · 2021-07-20T21:03:06Z

All committers have signed the CLA.

codecov · 2021-07-20T21:06:29Z

Codecov Report

Merging #2531 (eaf766d) into main (2e0551b) will increase coverage by 0.1%.
The diff coverage is 97.6%.

@@           Coverage Diff           @@
##            main   #2531     +/-   ##
=======================================
+ Coverage   99.9%   99.9%   +0.1%     
=======================================
  Files        291     291             
  Lines      26587   26644     +57     
=======================================
+ Hits       26544   26601     +57     
  Misses        43      43

Impacted Files	Coverage Δ
evalml/pipelines/components/__init__.py	`100.0% <ø> (ø)`
evalml/tests/component_tests/test_utils.py	`97.5% <50.0%> (ø)`
...alml/pipelines/components/transformers/__init__.py	`100.0% <100.0%> (ø)`
...elines/components/transformers/column_selectors.py	`100.0% <100.0%> (ø)`
...mponent_tests/test_column_selector_transformers.py	`100.0% <100.0%> (ø)`
evalml/tests/component_tests/test_components.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 2e0551b...eaf766d. Read the comment docs.

evalml/pipelines/components/transformers/column_selectors.py

freddyaboulton

@eccabay Looks good! Thanks for using the existing ColumnSelector abstraction!

One comment I'd like to resolve before merge is that if we make _check_input_for_columns a no-op we get semantic tag selection for free! I think that'll make this more flexible.

freddyaboulton · 2021-07-21T19:18:18Z

evalml/pipelines/components/transformers/column_selectors.py

+    needs_fitting = False
+
+    def _check_input_for_columns(self, X):
+        cols = self.parameters.get("columns") or []


What are your thoughts on making this a no-op?

If we do that, then we get selection by semantic tag for free. I feel like that would be useful for workflows that don't use the "standard" set of types. For example, a user might be interested in selecting all numeric features (including Age and AgeNullable) or all categorical features (CountryCode and Ordinal). It would also make it easy for users to select based on their own criteria if they set the tags correctly. For example, users may be interested in selecting based on whether a feature was created by another component or not.

Another argument for doing that is that check_input_for_columns was implemented because X.ww[cols] would error if the columns were not in the dataframe. However, Xww.select does not error out if the resulting dataframe is empty. I think it's ok if this returns an empty dataframe - maybe we can just raise a warning instead?

Agreed. I think this could definitely be a no-op, since it does allow us to select by semantic tags automatically.

Additionally,, currently it doesn't allow for selection if you use the capital string logical types (ie "NaturalLanguage" vs "natural_language")

I would think this should work since I can call X.ww.select() with both and get valid results

evalml/pipelines/components/transformers/column_selectors.py

bchen1116

Looking good! Agreed with Freddy, might be nice to not limit it to selecting by logical types. Left some comments on additional tests that could be helpful, as well as some clean up

docs/source/release_notes.rst

bchen1116 · 2021-07-21T20:35:29Z

evalml/pipelines/components/transformers/column_selectors.py

+    needs_fitting = False
+
+    def _check_input_for_columns(self, X):
+        cols = self.parameters.get("columns") or []


Agreed. I think this could definitely be a no-op, since it does allow us to select by semantic tags automatically.

Additionally,, currently it doesn't allow for selection if you use the capital string logical types (ie "NaturalLanguage" vs "natural_language")

I would think this should work since I can call X.ww.select() with both and get valid results

evalml/tests/component_tests/test_column_selector_transformers.py

ParthivNaresh · 2021-07-26T17:42:38Z

evalml/tests/component_tests/test_utils.py

-        n_components = 37
+        n_components = 38
    elif is_using_conda:
        n_components = 48


Putting a pin in this after our conversation during office hours 😅

Issue filed #2555

…479_dtype_columns

…into 2479_dtype_columns

ParthivNaresh · 2021-07-26T18:42:43Z

@eccabay I triggered build_conda_pkg on the PR just to verify that it fails without the bump in n_components. I went ahead and added +1 to is_using_conda and is_running_py_39_or_above.

@dsherry You might have to merge this to main since it won't pass the codecov test until we can address a solution for #2555

dsherry · 2021-07-29T16:41:54Z

evalml/pipelines/components/transformers/column_selectors.py

+    def _modify_columns(self, cols, X, y=None):
+        selected_columns = X.ww.select(cols)
+        if len(cols) > 0 and selected_columns.empty:
+            raise ValueError(f"Column(s) of type {cols} not found in input data")


This is the same type of error we raise in ColumnSelector._check_input_for_columns. Do we need to define it in two places? I think the intent with the current layout of ColumnSelector methods was for _check_input_for_columns to do this sort of validation, and for _modify_columns to only include logic to make modifications (i.e. dropping columns for the DropColumns transformer)

I think we should define this error only in _check_input_for_columns.

If you want the error to include the column types for SelectByType, but omit the column types for ColumnSelector and other subclasses, I would suggest something like:

Define a class attribute like _VALIDATION_ERROR_FORMAT_STRING = "Columns {columns} not found in input data."

Override the value of _VALIDATION_ERROR_FORMAT_STRING in the SelectByType class definition to "Columns {columns} of type {column_types} not found in input data"

Update ColumnSelector._check_input_for_columns to do

raise ValueError( self._VALIDATION_ERROR_FORMAT_STRING.format(**self.parameters) )

Update SelectByType._modify_columns to just run return X.ww.select(zip(columns, self.column_types)) (my example here prob not 100% correct, but close)

Alternatively, we could simplify this, just delete SelectColumns._modify_columns and not have the error message say anything about types.

Make sense?

dsherry

@eccabay looks good! I left some specific suggestions on how to reduce the complexity of this code change even further. Not blocking merge, feel free to merge this PR and get another up to address, if you are interested in doing so.

dsherry · 2021-07-29T16:45:26Z

@ParthivNaresh thanks for the heads up. Thankfully, since we made the codecov patch check optional, and that is the one which is going red on this PR, @eccabay doesn't need me to do the merge! The codecov project check is still green here.

…location

angela97lin

Thanks for cleaning this up and taking in all of our comments, Becca! This looks great 😁

I do have some final comments before this is merged regarding _check_input_for_columns--I'm not certain it's doing exactly what we want right now in SelectByType. It checks for column names via the columns parameter, but SelectByType checks for column types, so I'm not sure we can just use it as is?

angela97lin · 2021-07-29T21:07:32Z

evalml/pipelines/components/transformers/column_selectors.py

+
+    def transform(self, X, y=None):
+        X = infer_feature_types(X)
+        self._check_input_for_columns(X)


Interesting... does _check_input_for_columns do anything here? Since we don't define it, I'm assuming it uses ColumnSelector's, which has the line:

cols = self.parameters.get("columns") or []

That returns an empty list for this component and therefore doesn't do any checks, right?

It's true that that line will return an empty list and therefore the missing column name check will do nothing - that's by design here. This new bit of code should check if there are column types and if they match though:

col_types = self.parameters.get("column_types") if col_types: missing_cols = X.ww.select(col_types).empty

I agree, it does feel a little weird to do this here, I'm thinking it may make more sense to move this to check_input_for_columns within SelectByType itself.

angela97lin · 2021-07-29T21:12:58Z

evalml/pipelines/components/transformers/column_selectors.py

+    def __init__(self, column_types=None, random_seed=0, **kwargs):
+        parameters = {"column_types": column_types}
+        parameters.update(kwargs)
+        Transformer.__init__(


This, and the following comment, has me questioning if it's worth subclassing ColumnSelector rather than just Transformer; it seems like we're not gaining much value from ColumnSelector 😂

That's true, the way this is written there's not a lot of benefit to having SelectByType subclass this implementation of ColumnSelector. That being said, it still makes logical sense to have it be a subclass, since it does function to select columns. With that, I see this more as a reason to consider refactoring ColumnSelector rather than removing SelectByType as a subclass.

angela97lin · 2021-07-29T21:20:56Z

evalml/tests/component_tests/test_column_selector_transformers.py

    transformer = class_to_test(columns=["not in data"])
-    with pytest.raises(ValueError, match="'not in data' not found in input data"):
+    with pytest.raises(ValueError, match="not found in input data"):


This test doesn't actually test that a column type does not exist, but rather that a column does not exist! Since we're passing in the keyword columns, we trigger self._check_input_for_columns(X) to raise a ValueError, but don't we actually want to test the following:

transformer = SelectByType(column_types=["some fake column type"])

and see that the ValueError is raised? I think this is related to my previous comment about using _check_input_for_columns :)

eccabay added 6 commits July 20, 2021 16:11

Add SelectDtypeColumns class

fda7fb7

Test dtype selector

00519b5

Add dtype column selection using ww types directly

be8db44

lint fixes

a851502

Fix component tests to account for new class

2c90677

Add SelectDtypeColumns to api reference

479978c

Update release notes

0bb82e4

eccabay self-assigned this Jul 20, 2021

misc lint and test fixes

122f576

eccabay commented Jul 20, 2021

View reviewed changes

evalml/pipelines/components/transformers/column_selectors.py Outdated Show resolved Hide resolved

eccabay added 2 commits July 21, 2021 09:19

Merge branch 'main' into 2479_dtype_columns

50457d8

Attempt to fix codecov issue

72afc02

eccabay marked this pull request as ready for review July 21, 2021 18:24

eccabay requested review from ParthivNaresh, angela97lin, bchen1116, christopherbunn, chukarsten, dsherry, freddyaboulton and jeremyliweishih July 21, 2021 18:24

freddyaboulton approved these changes Jul 21, 2021

View reviewed changes

bchen1116 requested changes Jul 21, 2021

View reviewed changes

Address PR comments

d469c14

eccabay requested a review from bchen1116 July 22, 2021 13:58

eccabay added 3 commits July 22, 2021 10:09

Small name and description fixes

422fd0a

Merge branch 'main' into 2479_dtype_columns

a977f65

Fix release notes

eb91650

eccabay mentioned this pull request Jul 26, 2021

Define consistent behavior for empty DataFrames in pipelines #2554

Open

ParthivNaresh reviewed Jul 26, 2021

View reviewed changes

eccabay and others added 5 commits July 26, 2021 13:50

Rename SelectByTypeorTag to SelectByType

4a50c5d

trigger build conda package

aee6961

Throw an exception instead of warning on empty df

48a131c

Merge branch '2479_dtype_columns' of github.com:alteryx/evalml into 2…

eac4e0f

…479_dtype_columns

lint fixes

1ca89b4

eccabay requested a review from dsherry July 26, 2021 18:37

ParthivNaresh added 2 commits July 26, 2021 14:42

re-add paths, bump n_components

4de58f9

Merge branch '2479_dtype_columns' of ssh://github.com/alteryx/evalml …

6bf49c6

…into 2479_dtype_columns

ParthivNaresh and others added 2 commits July 26, 2021 14:43

Merge branch 'main' into 2479_dtype_columns

625a8ab

Merge branch 'main' into 2479_dtype_columns

e0e9b56

dsherry reviewed Jul 29, 2021

View reviewed changes

dsherry approved these changes Jul 29, 2021

View reviewed changes

eccabay added 3 commits July 29, 2021 16:02

Move columns arg to column_types in SelectByType and fix error logic …

1b85325

…location

Merge branch 'main' into 2479_dtype_columns

6fde69a

lint fixes

6c979b3

angela97lin reviewed Jul 29, 2021

View reviewed changes

Merge branch 'main' into 2479_dtype_columns

5124e58

eccabay mentioned this pull request Jul 30, 2021

Refactoring ColumnSelector and subclasses #2574

Open

eccabay added 5 commits July 30, 2021 13:44

Move type validation logic to subclass

d479200

more and more lint

90833e4

Merge branch 'main' into 2479_dtype_columns

f6cd8fb

Merge branch 'main' into 2479_dtype_columns

142e62e

Increase number of total components

eaf766d

eccabay merged commit 53eaf22 into main Jul 30, 2021

eccabay deleted the 2479_dtype_columns branch July 30, 2021 19:42

chukarsten mentioned this pull request Aug 3, 2021

Release v0.30.0 #2584

Merged

Conversation

eccabay commented Jul 20, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

dsherry Jul 29, 2021 • 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!

dsherry left a comment

Choose a reason for hiding this comment

Uh oh!

dsherry commented Jul 29, 2021

Uh oh!

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

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!

8 participants

CLAassistant commented Jul 20, 2021 •

edited

Loading

codecov bot commented Jul 20, 2021 •

edited

Loading

ParthivNaresh commented Jul 26, 2021 •

edited

Loading

dsherry Jul 29, 2021 •

edited

Loading