Add null rows check to `HighlyNullDataCheck` #2222

jeremyliweishih · 2021-05-03T20:20:28Z

…acheck

jeremyliweishih · 2021-05-04T20:43:42Z

evalml/data_checks/highly_null_rows_data_check.py

+class HighlyNullRowsDataCheck(DataCheck):
+    """Checks if there are any highly-null rows in the input."""
+
+    def __init__(self, pct_null_threshold=0.5):


I set default to 0.5 (as opposed to 0.95) because I think this can be more aggressive on removing individual samples/rows than removing a whole column in HighlyNullDataCheck.

codecov · 2021-05-04T22:20:06Z

Codecov Report

Merging #2222 (8c96390) into main (40a8765) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##             main    #2222     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         280      280             
  Lines       24322    24357     +35     
=========================================
+ Hits        24300    24335     +35     
  Misses         22       22

Impacted Files	Coverage Δ
evalml/data_checks/default_data_checks.py	`100.0% <ø> (ø)`
evalml/data_checks/data_check_action_code.py	`100.0% <100.0%> (ø)`
evalml/data_checks/data_check_message_code.py	`100.0% <100.0%> (ø)`
evalml/data_checks/highly_null_data_check.py	`100.0% <100.0%> (ø)`
...tests/data_checks_tests/test_data_check_message.py	`100.0% <100.0%> (ø)`
evalml/tests/data_checks_tests/test_data_checks.py	`100.0% <100.0%> (ø)`
...s/data_checks_tests/test_highly_null_data_check.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 40a8765...8c96390. Read the comment docs.

chukarsten

Great work here, Jeremy. I think we should merge the logic for the 0% and the rest of the percentage empty in the data check .validate(). Unless there's a really compelling reason for this additional complexity, I think it'll be good to drop the if branching and merge them. Additionally, we might want to talk about adding test coverage for other null types? This looks great though. Really like the tests - very readable.

evalml/data_checks/highly_null_rows_data_check.py

evalml/tests/data_checks_tests/test_highly_null_rows_data_check.py

freddyaboulton

@jeremyliweishih Looks great! I think this is good to merge pending @chukarsten 's comments.

evalml/data_checks/default_data_checks.py

freddyaboulton · 2021-05-05T15:17:11Z

evalml/data_checks/highly_null_rows_data_check.py

+from evalml.utils import _convert_woodwork_types_wrapper, infer_feature_types
+
+
+class HighlyNullRowsDataCheck(DataCheck):


I wonder if we can share some code with HighlyNullDataCheck. The only computational difference I see is

(X.isnull().mean()).to_dict() vs (X.isnull().mean(axis=1)).to_dict().

I'd be happy to punt on this though, doesn't seem super pressing.

I thought about this as well but I felt that it fits better into DefaultDataChecks and DataChecks better as separate data checks. I don't feel too strongly on this though - happy to see what others think!

The two options presented so far:

Update the existing HighlyNullDataCheck to also perform a null row check. Don't define a new data check.

Define a separate HighlyNullRowsDataCheck as this PR currently does.

The argument for option 1: Simpler. No duplicated impl code. No need to add plumbing for new check up the stack (outside EvalML). Potential for better performance on larger datasets because we'd only need to call X.isnull() once.
The argument for option 2: Easier for users to selectively only run either the col or row null checks in isolation instead of always running both.

Anything else?

I agree with @freddyaboulton here that option 1 is better.

bchen1116

This implementation looks good! I did have a question that I think we should address, whether in this PR or in a future issue.

Currently, we find the null values with the original dataset, but I can see issues if we have a dataset like:

X = pd.DataFrame({"col_1": [None, None, None],
                  "col_2": [None, 1, 2],
                  "col_3": [1, 2, None]})

Then the data check for null columns would suggest dropping col 1, and the data check for null rows would drop cols 2 and 3. I'm curious if we want to drop the null columns BEFORE finding the rows that satisfy this data check.

It's mainly for larger datasets, where there might be 1000+ columns, and maybe half have empty columns. These two highlyNull datachecks would then drop all the rows and columns, which doesn't seem like the behavior we want.

Just something that could raise problems in the future. Requested changes since I wanted to have this discussion before merging!

docs/source/release_notes.rst

…acheck

jeremyliweishih · 2021-05-05T19:07:11Z

@bchen1116 good points! I'm not sure if there has been discussions on the order of operations with data check actions but I do like your suggestion. A solution now we could do would be to bump pct_null_threshold up so that the case you brought up won't happen. I can bump it up to something like 0.75 which may be more reasonable. What do you think?

bchen1116 · 2021-05-05T19:18:34Z

@jeremyliweishih yeah, I think that's a good short-term solution! Might need to discuss further for a longer-term solution

bchen1116

Nice!

stale

dsherry

@jeremyliweishih I'd like you to change the following:

Add to existing HighlyNullDataCheck instead of creating a separate data check
Return a single DataCheckWarning when one or more rows are highly null, instead of one warning for each row
In the warning details, save info about which rows are highly null and what the percentage was using a single pandas Series instead of a dict.

dsherry · 2021-05-06T15:25:28Z

evalml/data_checks/data_check_action_code.py

@@ -7,5 +7,8 @@ class DataCheckActionCode(Enum):
    DROP_COL = "drop_col"
    """Action code for dropping a column."""

+    DROP_ROW = "drop_row"
+    """Action code for dropping a row."""


Let's call this DROP_ROWS since we'll rarely be dropping a single row. Relates to my comments about the impl.

dsherry · 2021-05-06T15:36:57Z

evalml/data_checks/highly_null_rows_data_check.py

+from evalml.utils import _convert_woodwork_types_wrapper, infer_feature_types
+
+
+class HighlyNullRowsDataCheck(DataCheck):


The two options presented so far:

Update the existing HighlyNullDataCheck to also perform a null row check. Don't define a new data check.

Define a separate HighlyNullRowsDataCheck as this PR currently does.

The argument for option 1: Simpler. No duplicated impl code. No need to add plumbing for new check up the stack (outside EvalML). Potential for better performance on larger datasets because we'd only need to call X.isnull() once.
The argument for option 2: Easier for users to selectively only run either the col or row null checks in isolation instead of always running both.

Anything else?

I agree with @freddyaboulton here that option 1 is better.

dsherry · 2021-05-06T15:44:30Z

evalml/data_checks/highly_null_rows_data_check.py

+        X = _convert_woodwork_types_wrapper(X.to_dataframe())
+
+        percent_null = (X.isnull().mean(axis=1)).to_dict()
+        highly_null_rows = {key: value for key, value in percent_null.items() if value >= self.pct_null_threshold and value != 0}


Instead of using a dictionary here, what if we represented this like so:

percent_null = X.isnull().mean(axis=1) highly_null = percent_null[percent_null >= self.pct_null_threshold] results["warnings"].extend([DataCheckWarning(..., details={"row": row_name, "pct_null_cols": highly_null})])

For large datasets I think this is a more efficient way to share this info. Since we don't reset the index above, pandas will track keep track of it for us, so the indices of the rows which are highly null can be obtained by grabbing "pct_null_cols" from the warning details and running pct_null_cols.index

dsherry · 2021-05-06T16:30:58Z

evalml/data_checks/highly_null_rows_data_check.py

+                                                     data_check_name=self.name,
+                                                     message_code=DataCheckMessageCode.HIGHLY_NULL_ROWS,
+                                                     details={"row": row_name, "pct_null_cols": highly_null_rows[row_name]}).to_dict()
+                                    for row_name in highly_null_rows])


This approach will not do well on large and/or sparse datasets. Consider the case when the data is 100k rows x 100 cols and each feature is 1% missing values, but also the row position of those missing values has 0% overlap. In that case we'd end up with 100k DataCheckWarning message instances in the list here.

We should certainly still allow the caller to see which rows were null, but I don't think we should require them to process each row as a separate warning.

Instead let's return one DataCheckWarning for the entire dataset.

if len(highly_null) > 0: warning_msg = f"{len(highly_null)} out of {len(X)} rows are more than {self.pct_null_threshold*100}% null" results["warnings"].append( DataCheckWarning(message=warning_msg, data_check_name=self.name, message_code=DataCheckMessageCode.HIGHLY_NULL_ROWS, details={"pct_null_cols": highly_null})

where I'm using highly_null from my other comment above

…acheck

dsherry

@jeremyliweishih thanks, nice going! 🚢 😁

dsherry · 2021-05-13T19:34:34Z

evalml/data_checks/highly_null_data_check.py

+            results["warnings"].append(DataCheckWarning(message=warning_msg,
+                                                        data_check_name=self.name,
+                                                        message_code=DataCheckMessageCode.HIGHLY_NULL_ROWS,
+                                                        details={"pct_null_cols": highly_null_rows}).to_dict())


Thanks! Looks great

dsherry · 2021-05-13T19:39:15Z

evalml/data_checks/highly_null_data_check.py


        Arguments:
            pct_null_threshold(float): If the percentage of NaN values in an input feature exceeds this amount,
-                that feature will be considered highly-null. Defaults to 0.95.
+                that column/row will be considered highly-null. Defaults to 0.95.


Ah, I do think it would be helpful to have a separate threshold for rows vs columns.

I filed #2270 to track that separately.

jeremyliweishih added 3 commits May 3, 2021 16:19

Add highly null row data check

684e419

Merge branch 'main' of github.com:alteryx/evalml into js_2220_row_dat…

49f55fb

…acheck

RL

6182bad

jeremyliweishih changed the title ~~Add HighlyNullRowDataCheck~~ Add HighlyNullRowsDataCheck May 3, 2021

jeremyliweishih added 5 commits May 3, 2021 16:31

Add to API reference

03cb3b7

Fix doctest

faa569f

Merge branch 'main' of github.com:alteryx/evalml into js_2220_row_dat…

7918034

…acheck

Add to default datachecks

752b2c4

Merge branch 'main' of github.com:alteryx/evalml into js_2220_row_dat…

1cc5bf3

…acheck

jeremyliweishih commented May 4, 2021

View reviewed changes

Fix tests

85a540e

jeremyliweishih marked this pull request as ready for review May 5, 2021 13:02

auto-assign bot assigned jeremyliweishih May 5, 2021

Fix docstring

ffa7b5a

jeremyliweishih requested review from angela97lin, dsherry, bchen1116, chukarsten, freddyaboulton and ParthivNaresh and removed request for dsherry May 5, 2021 14:42

chukarsten previously requested changes May 5, 2021

View reviewed changes

evalml/data_checks/highly_null_rows_data_check.py Outdated Show resolved Hide resolved

evalml/tests/data_checks_tests/test_highly_null_rows_data_check.py Outdated Show resolved Hide resolved

evalml/tests/data_checks_tests/test_highly_null_rows_data_check.py Outdated Show resolved Hide resolved

freddyaboulton approved these changes May 5, 2021

View reviewed changes

bchen1116 requested changes May 5, 2021

View reviewed changes

docs/source/release_notes.rst Outdated Show resolved Hide resolved

jeremyliweishih added 2 commits May 5, 2021 15:02

Merge branch 'main' of github.com:alteryx/evalml into js_2220_row_dat…

813fa4f

…acheck

Fix RL

2a3a1ef

merge > 0 and 0 case

41dc0fb

jeremyliweishih added 2 commits May 5, 2021 15:31

Bump default to 0.75

4e8139e

Test with None and np.nan

823ee83

jeremyliweishih mentioned this pull request May 5, 2021

Rename HighlyNullDataCheck to HighlyNullColumnsDataCheck #2233

Closed

bchen1116 approved these changes May 5, 2021

View reviewed changes

jeremyliweishih requested a review from chukarsten May 5, 2021 20:44

dsherry suggested changes May 6, 2021

View reviewed changes

jeremyliweishih added 6 commits May 10, 2021 09:55

Merge branch 'main' of github.com:alteryx/evalml into js_2220_row_dat…

2bdc3a3

…acheck

Simplify highly null data check

26d12fd

Combine row and cols check

b4e7e88

Fix default datacheck tests

c65a1e7

Fix removals

23bea89

Add null row example to default data checks

5e366f2

jeremyliweishih requested a review from dsherry May 10, 2021 21:41

jeremyliweishih changed the title ~~Add HighlyNullRowsDataCheck~~ Add null rows check to HighlyNullDataCheck May 12, 2021

Merge branch 'main' of github.com:alteryx/evalml into js_2220_row_dat…

1aab69a

…acheck

dsherry mentioned this pull request May 13, 2021

Highly null data check: separate thresholds for pct null cols vs rows #2270

Closed

dsherry approved these changes May 13, 2021

View reviewed changes

Merge branch 'main' into js_2220_row_datacheck

8c96390

jeremyliweishih merged commit 3a7ee46 into main May 13, 2021

chukarsten mentioned this pull request May 17, 2021

Release v0.24.1 #2279

Merged

freddyaboulton deleted the js_2220_row_datacheck branch May 13, 2022 15:18

Add null rows check to `HighlyNullDataCheck` #2222

Add null rows check to `HighlyNullDataCheck` #2222

jeremyliweishih commented May 3, 2021

jeremyliweishih May 4, 2021 •

edited

codecov bot commented May 4, 2021 •

edited

chukarsten left a comment

freddyaboulton left a comment

freddyaboulton May 5, 2021

jeremyliweishih May 5, 2021

dsherry May 6, 2021

bchen1116 left a comment •

edited

jeremyliweishih commented May 5, 2021

bchen1116 commented May 5, 2021

bchen1116 left a comment

dsherry left a comment

dsherry May 6, 2021

dsherry May 6, 2021

dsherry May 6, 2021

dsherry May 6, 2021

dsherry left a comment

dsherry May 13, 2021

dsherry May 13, 2021

		from evalml.utils import _convert_woodwork_types_wrapper, infer_feature_types


		class HighlyNullRowsDataCheck(DataCheck):

Add null rows check to HighlyNullDataCheck #2222

Add null rows check to HighlyNullDataCheck #2222

Conversation

jeremyliweishih commented May 3, 2021

jeremyliweishih May 4, 2021 • edited

Choose a reason for hiding this comment

codecov bot commented May 4, 2021 • edited

Codecov Report

chukarsten left a comment

Choose a reason for hiding this comment

freddyaboulton 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

bchen1116 left a comment • edited

Choose a reason for hiding this comment

jeremyliweishih commented May 5, 2021

bchen1116 commented May 5, 2021

bchen1116 left a comment

Choose a reason for hiding this comment

dsherry 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

dsherry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add null rows check to `HighlyNullDataCheck` #2222

Add null rows check to `HighlyNullDataCheck` #2222

jeremyliweishih May 4, 2021 •

edited

codecov bot commented May 4, 2021 •

edited

bchen1116 left a comment •

edited