Add Model Understanding Tests - #2286
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2286 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 280 280
Lines 24392 24410 +18
=========================================
+ Hits 24369 24387 +18
Misses 23 23
Continue to review full report at Codecov.
|
dsherry
added
task
dsherry
left a comment
dsherry
left a comment
There was a problem hiding this comment.
You rock, thanks for this! This will cover our butts while we work on adding coverage for automl --> model understanding to our release test suite.
I just had one blocking comment.
angela97lin
left a comment
angela97lin
left a comment
There was a problem hiding this comment.
Looks good besides what @dsherry pointed out, but I think it could be worthwhile to try to all estimators and single out catboost / EN. Then, when those issues are resolved it'll be easier to catch here and update the tests accordingly.
ParthivNaresh
left a comment
ParthivNaresh
left a comment
There was a problem hiding this comment.
Looks great, no other comments besides the changes already requested!
| pipeline.fit(X, y) | ||
| report = explain_predictions(pipeline, X, y, indices_to_explain=[0], output_format="dataframe", top_k_features=4) | ||
| assert report['feature_names'].isnull().sum() == 0 | ||
| assert report['feature_values'].isnull().sum() == 0 |
There was a problem hiding this comment.
Yep this seems reasonable to me. We have other tests I believe which check that the output of prediction explanations is what we expect it to be. This test is just checking that something came out which matches the form we expect. 👍
| pipeline = BinaryClassificationPipeline(component_graph=["Imputer", "One Hot Encoder", "DateTime Featurization Component", "SMOTENC Oversampler", estimator]) | ||
| pipeline.fit(X=X, y=y) | ||
| pipeline.predict(X) | ||
| importance = calculate_permutation_importance(pipeline, X, y, objective="Log Loss Binary") |
There was a problem hiding this comment.
@bchen1116 this can be a long-running computation. How long does this unit test take locally?
If its longer than a couple seconds total I suggest just doing it for one estimator. IMO I think that's acceptable because the goal for these tests is to prove that using a component which modifies the target during training (oversampler) doesn't break the model understanding tools, so iterating over the estimators is nice but not a requirement.
There was a problem hiding this comment.
Yep! Running all locally took ~36 seconds, while running with one estimator took ~4.5 seconds. Will change to using just 1 estimator!
4 participants
Address comments from this
We can't do
prediction_explanationsfor Catboost or Elastic Net Classifier, and we already have issues related to the bugs here and here