Clean up and add woodwork support for more methods by angela97lin · Pull Request #1544 · alteryx/evalml

angela97lin · 2020-12-10T21:08:22Z

angela97lin · 2020-12-10T21:09:11Z

    return conf_mat


+def graph_confusion_matrix(y_true, y_pred, normalize_method='true', title_addition=None):


Just moved this up, to be spatially connected with the other confusion matrix-related methods 😆

codecov · 2020-12-10T23:11:16Z

Codecov Report

Merging #1544 (a37ab4f) into main (0f1f1c6) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##             main    #1544     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         234      234             
  Lines       16742    16773     +31     
=========================================
+ Hits        16734    16765     +31     
  Misses          8        8

Impacted Files	Coverage Δ
evalml/objectives/cost_benefit_matrix.py	`100.0% <ø> (ø)`
evalml/objectives/fraud_cost.py	`100.0% <ø> (ø)`
evalml/objectives/lead_scoring.py	`100.0% <ø> (ø)`
evalml/tests/conftest.py	`100.0% <ø> (ø)`
evalml/model_understanding/graphs.py	`99.7% <100.0%> (+0.1%)`	⬆️
...lml/tests/model_understanding_tests/test_graphs.py	`100.0% <100.0%> (ø)`
.../tests/objective_tests/test_cost_benefit_matrix.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 0f1f1c6...a37ab4f. Read the comment docs.

angela97lin · 2020-12-11T17:27:33Z

    "from evalml.demos.churn import load_churn\n",
    "from evalml.preprocessing import split_data\n",
    "X, y = load_churn()\n",
+    "X = X.set_types({'PaymentMethod':'Categorical', 'Contract': 'Categorical'}) # Update data types Woodwork did not correctly infer\n",


Woodwork inferred these as natural language, causing a text featurizer to be added and the runtime to significantly increase so manually updating here.

Nice! I don't think there are any other places left in the docs where we unintentionally use a text featurizer.

angela97lin · 2020-12-11T17:28:52Z

    y_predicted = _convert_to_woodwork_structure(y_predicted)
-    y_true = _convert_woodwork_types_wrapper(y_true.to_series())
-    y_predicted = _convert_woodwork_types_wrapper(y_predicted.to_series())
-    if isinstance(y_true, pd.Series):


Removed, since _convert_woodwork_types_wrapper will always return pandas so no need for if statement

angela97lin · 2020-12-11T17:31:38Z

    assert isinstance(conf_mat, pd.DataFrame)

-    conf_mat = confusion_matrix(y_true, y_predicted, normalize_method='pred')
-    conf_mat_expected = np.array([[2 / 3.0, np.nan, 0], [0, np.nan, 1 / 3.0], [1 / 3.0, np.nan, 2 / 3.0]])


Not sure why this wasn't caught / if this was expected behavior? But based on our code for confusion_matrix, if any of the axis calculations while normalizing have a sum of zero, then we should raise an error (test_normalize_confusion_matrix_error); Updated values of matrix here so no error is thrown.

There's a bug in the implementation/test 😬 The warning is only raised if the input to normalize_confusion_matrix is a numpy array but in confusion_matrix we always convert to a dataframe. This means there is no warning to catch so we don't raise the ValueError.

Ah you're absolutely right, it's only numpy that only displayed this warning 😢

freddyaboulton

@angela97lin Thanks! I think you found a bug in our implementation of normalize_confusion_matrix hehe

freddyaboulton · 2020-12-11T18:13:58Z

    "from evalml.demos.churn import load_churn\n",
    "from evalml.preprocessing import split_data\n",
    "X, y = load_churn()\n",
+    "X = X.set_types({'PaymentMethod':'Categorical', 'Contract': 'Categorical'}) # Update data types Woodwork did not correctly infer\n",


Nice! I don't think there are any other places left in the docs where we unintentionally use a text featurizer.

freddyaboulton · 2020-12-11T18:55:47Z

+        y (ww.DataColumn, pd.Series): Target values to compare predictions against.
+        dates (ww.DataColumn, pd.Series): Dates corresponding to target values and predictions.

    Returns:


I think we need to convert dates and y to pandas here. I don't think reset_index will work on a DataColumn.

@freddyaboulton Ah, you're right. Looks like there's no tests for this 🤔 I'll add something and check for ww!

freddyaboulton · 2020-12-11T19:30:09Z

    assert isinstance(conf_mat, pd.DataFrame)

-    conf_mat = confusion_matrix(y_true, y_predicted, normalize_method='pred')
-    conf_mat_expected = np.array([[2 / 3.0, np.nan, 0], [0, np.nan, 1 / 3.0], [1 / 3.0, np.nan, 2 / 3.0]])


There's a bug in the implementation/test 😬 The warning is only raised if the input to normalize_confusion_matrix is a numpy array but in confusion_matrix we always convert to a dataframe. This means there is no warning to catch so we don't raise the ValueError.

… 1292_utils_graphs_ww

angela97lin added 2 commits December 8, 2020 15:44

init

e55a47c

cleanup

fa71096

angela97lin added this to the December 2020 milestone Dec 10, 2020

angela97lin self-assigned this Dec 10, 2020

angela97lin commented Dec 10, 2020

View reviewed changes

angela97lin added 5 commits December 10, 2020 16:40

update types in notebook

879f893

linting

44452bc

release notes

0271cd1

Merge branch 'main' into 1292_utils_graphs_ww

e9f4bbe

fix tests

e1f84ac

angela97lin marked this pull request as ready for review December 11, 2020 16:42

angela97lin commented Dec 11, 2020

View reviewed changes

cleanup

7aa5a35

angela97lin requested review from ParthivNaresh, bchen1116, dsherry, freddyaboulton and jeremyliweishih and removed request for dsherry December 11, 2020 17:31

freddyaboulton approved these changes Dec 11, 2020

View reviewed changes

angela97lin added 6 commits December 14, 2020 12:46

Merge branch 'main' into 1292_utils_graphs_ww

8ba3f88

add tests

97bf888

Merge branch '1292_utils_graphs_ww' of github.com:alteryx/evalml into…

a502ad7

… 1292_utils_graphs_ww

Merge branch 'main' into 1292_utils_graphs_ww

026724d

fix test

83d2f47

Merge branch '1292_utils_graphs_ww' of github.com:alteryx/evalml into…

a37ab4f

… 1292_utils_graphs_ww

angela97lin merged commit 3698425 into main Dec 14, 2020

angela97lin deleted the 1292_utils_graphs_ww branch December 14, 2020 21:57

dsherry mentioned this pull request Dec 29, 2020

Release v0.17.0 #1623

Merged

		return conf_mat


		def graph_confusion_matrix(y_true, y_pred, normalize_method='true', title_addition=None):

Uh oh!

Conversation

angela97lin commented Dec 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Dec 10, 2020 • 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!

freddyaboulton Dec 11, 2020 • 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!

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!

freddyaboulton Dec 11, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Dec 10, 2020 •

edited

Loading

freddyaboulton Dec 11, 2020 •

edited

Loading

freddyaboulton Dec 11, 2020 •

edited

Loading