-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wrap call to scikit-learn's partial dependence method in a try/finally block #1232
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1232 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 196 196
Lines 12193 12206 +13
=======================================
+ Hits 12184 12197 +13
Misses 9 9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -689,6 +689,20 @@ def test_partial_dependence_problem_types(problem_type, X_y_binary, X_y_multi, X | |||
pipeline.feature_importances_ | |||
|
|||
|
|||
@patch('evalml.model_understanding.graphs.sk_partial_dependence') | |||
def test_partial_dependence_error_still_deletes_attributes(mock_part_dep, X_y_binary, logistic_regression_binary_pipeline_class): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angela97lin I think this looks great!
# Delete scikit-learn attributes that were temporarily set | ||
del pipeline._estimator_type | ||
del pipeline.feature_importances_ | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have to repeat this pattern in the future, it'd be cool to use a context manager!
From @dsherry's comment in #1150, wrap call to scikit-learn's partial dependence method in a try/finally block so that we still delete the set attributes even if call to scikit-learn's partial dependence method fails or errors out.