Standardize feature importance for estimators #3305

angela97lin · 2022-02-04T05:53:24Z

codecov · 2022-02-04T05:57:54Z

Codecov Report

Merging #3305 (194e60c) into main (e67430c) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3305     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        327     327             
  Lines      31814   31840     +26     
=======================================
+ Hits       31689   31715     +26     
  Misses       125     125

Impacted Files	Coverage Δ
evalml/model_understanding/graphs.py	`100.0% <100.0%> (ø)`
...ents/estimators/classifiers/baseline_classifier.py	`100.0% <100.0%> (ø)`
...ents/estimators/classifiers/catboost_classifier.py	`100.0% <100.0%> (ø)`
...ts/estimators/classifiers/elasticnet_classifier.py	`100.0% <100.0%> (ø)`
...tors/classifiers/logistic_regression_classifier.py	`100.0% <100.0%> (ø)`
...nents/estimators/classifiers/xgboost_classifier.py	`100.0% <100.0%> (ø)`
...valml/pipelines/components/estimators/estimator.py	`100.0% <100.0%> (ø)`
...onents/estimators/regressors/catboost_regressor.py	`100.0% <100.0%> (ø)`
...ents/estimators/regressors/elasticnet_regressor.py	`100.0% <100.0%> (ø)`
...tors/regressors/exponential_smoothing_regressor.py	`100.0% <100.0%> (ø)`
... and 5 more

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 e67430c...194e60c. Read the comment docs.

jeremyliweishih

Some food for thought but not blocking!

jeremyliweishih · 2022-02-07T21:48:38Z

evalml/model_understanding/graphs.py

@@ -1526,7 +1526,8 @@ def get_linear_coefficients(estimator, features=None):
            "before using this estimator."
        )
    coef_ = estimator.feature_importance
-    coef_ = pd.Series(coef_, name="Coefficients", index=features)
+    coef_ = pd.Series(coef_, name="Coefficients")


Do we need all the pd.Series calls with this call here?

Nope since coef_ should be a pd.Series object now. Though we still want the name, so I can update this line to set the name instead.

jeremyliweishih · 2022-02-07T21:53:50Z

evalml/tests/component_tests/test_estimators.py

@@ -359,3 +359,30 @@ def test_estimator_fit_predict_and_predict_proba_respect_custom_indices(
        )
    X_pred = estimator.predict(X)
    pd.testing.assert_index_equal(X_original_index, X_pred.index, check_names=True)
+
+
+@pytest.mark.parametrize("estimator_class", _all_estimators())


Should we delete any of the previous tests? I think we could remove all the tests don't check equality (i.e the estimators that don't support feature importance) since there's some redundancy between these tests.

Indeed, I can see this going both ways. I can see tests here in test_estimators existing for the purpose of determining that no feature_importance() sends back NaN. If we wanted to do any assertion on the shapes of feature importances relative to the input datasets, now and here would be the place to do it. I think if we wanted, for some reason, to test the specific feature_importance() of an individual estimator and make sure that it calculates the right values, we'd want to do it in that specific module.

I like what @chukarsten is saying here, where this is more general and just tests that we don't return nan values / the shapes are expected. Specific modules will test for expected values (since many of the feature importances are calculated from an external library) or assert that we can't calculate it.

chukarsten

Thanks for standardizing and reducing my OCD :)

chukarsten · 2022-02-09T17:25:34Z

evalml/tests/component_tests/test_estimators.py

@@ -359,3 +359,30 @@ def test_estimator_fit_predict_and_predict_proba_respect_custom_indices(
        )
    X_pred = estimator.predict(X)
    pd.testing.assert_index_equal(X_original_index, X_pred.index, check_names=True)
+
+
+@pytest.mark.parametrize("estimator_class", _all_estimators())


Indeed, I can see this going both ways. I can see tests here in test_estimators existing for the purpose of determining that no feature_importance() sends back NaN. If we wanted to do any assertion on the shapes of feature importances relative to the input datasets, now and here would be the place to do it. I think if we wanted, for some reason, to test the specific feature_importance() of an individual estimator and make sure that it calculates the right values, we'd want to do it in that specific module.

ParthivNaresh

Looks great, just left a comment about whether we want a Series name to be part of the standardization in case people call it from the estimator directly and not from model understanding.

ParthivNaresh · 2022-02-14T14:31:43Z

evalml/model_understanding/graphs.py

@@ -1526,7 +1526,8 @@ def get_linear_coefficients(estimator, features=None):
            "before using this estimator."
        )
    coef_ = estimator.feature_importance
-    coef_ = pd.Series(coef_, name="Coefficients", index=features)
+    coef_ = pd.Series(coef_, name="Coefficients")
+    coef_.index = features


Just out of curiosity, why did we separate these?

I couldn't get it to work with them together, because of pandas behavior so it reindexes after and returns all NaNs if put on the same line (or that's my rough understanding)?https://pandas.pydata.org/docs/reference/api/pandas.Series.html

ParthivNaresh · 2022-02-14T14:37:21Z

evalml/tests/component_tests/test_exponential_smoothing_regressor.py

@@ -119,7 +119,9 @@ def test_feature_importance(ts_data):
    regressor = ExponentialSmoothingRegressor()
    with patch.object(regressor, "_component_obj"):
        regressor.fit(X, y)
-        assert regressor.feature_importance == np.zeros(1)
+        pd.testing.assert_series_equal(
+            regressor.feature_importance, pd.Series(np.zeros(1))


Do we want to add a name to the feature importance series?

Filed #3330 that we can use to track this! :)

init

dee52ae

angela97lin self-assigned this Feb 4, 2022

angela97lin added 4 commits February 7, 2022 01:47

oops fix some tests

352fe2d

Merge branch 'main' into 3183_feature_importance

3334362

update test

12580e0

release notes

82c39a1

angela97lin marked this pull request as ready for review February 7, 2022 20:43

angela97lin requested review from chukarsten, bchen1116, eccabay, dsherry, freddyaboulton, jeremyliweishih, christopherbunn and ParthivNaresh February 7, 2022 20:44

jeremyliweishih approved these changes Feb 7, 2022

View reviewed changes

chukarsten approved these changes Feb 9, 2022

View reviewed changes

ParthivNaresh approved these changes Feb 14, 2022

View reviewed changes

angela97lin mentioned this pull request Feb 14, 2022

Add a name to the pandas series returned for feature importance #3330

Open

angela97lin added 2 commits February 14, 2022 16:36

update line to just update name

8895c31

Merge branch 'main' into 3183_feature_importance

194e60c

angela97lin merged commit 95aa7e2 into main Feb 15, 2022

angela97lin deleted the 3183_feature_importance branch February 15, 2022 03:57

chukarsten mentioned this pull request Feb 18, 2022

Release v0.45.0 #3344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize feature importance for estimators #3305

Standardize feature importance for estimators #3305

angela97lin commented Feb 4, 2022

codecov bot commented Feb 4, 2022 •

edited

Loading

jeremyliweishih left a comment

jeremyliweishih Feb 7, 2022

angela97lin Feb 14, 2022

jeremyliweishih Feb 7, 2022

chukarsten Feb 9, 2022

angela97lin Feb 14, 2022

chukarsten left a comment

chukarsten Feb 9, 2022

ParthivNaresh left a comment •

edited

Loading

ParthivNaresh Feb 14, 2022

angela97lin Feb 14, 2022

ParthivNaresh Feb 14, 2022

angela97lin Feb 14, 2022

Standardize feature importance for estimators #3305

Standardize feature importance for estimators #3305

Conversation

angela97lin commented Feb 4, 2022

codecov bot commented Feb 4, 2022 • edited Loading

Codecov Report

jeremyliweishih 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

Choose a reason for hiding this comment

chukarsten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParthivNaresh left a comment • edited Loading

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

codecov bot commented Feb 4, 2022 •

edited

Loading

ParthivNaresh left a comment •

edited

Loading