Skip to content
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

Raise a custom exception for partial dependence errors #2604

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

freddyaboulton
Copy link
Contributor

@freddyaboulton freddyaboulton commented Aug 6, 2021

Pull Request Description

Fixes #2539

Design document here .


After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of docs/source/release_notes.rst to include this pull request by adding :pr:123.

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #2604 (53b932b) into main (3a00d47) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2604     +/-   ##
=======================================
+ Coverage   99.9%   99.9%   +0.1%     
=======================================
  Files        295     295             
  Lines      26848   26894     +46     
=======================================
+ Hits       26802   26848     +46     
  Misses        46      46             
Impacted Files Coverage Δ
evalml/exceptions/__init__.py 100.0% <ø> (ø)
evalml/exceptions/exceptions.py 100.0% <100.0%> (ø)
evalml/model_understanding/graphs.py 100.0% <100.0%> (ø)
...del_understanding_tests/test_partial_dependence.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 3a00d47...53b932b. Read the comment docs.

raise ValueError(
"Too many features given to graph_partial_dependence. Only one or two-way partial "
"dependence is supported."
try:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this try unfortunately makes the diff seem bigger than it is. I think it's important to catch all possible errors that this can raise so that users don't have to resort to except Exception or except PartialDependenceError, ValueError and hope that'll catch everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an outstanding issue to clean up partial dependence (#2502) so maybe this will be cleaner once we redesign this with this requirement in mind.

Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very awesome, great work. Thanks for consolidating all these exceptions and making it super tidy. I think this will also help downstream consumers of EvalML a LOT.

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for cleaning this up and making it easier to tell users what the issue is. The error docs don't appear on the docs though, unless you explicitly type it into the search?

code=PartialDependenceErrorCode.COMPUTED_PERCENTILES_TOO_CLOSE,
)
else:
raise e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so this error isn't necessarily due to Partial Dependence, so it doesn't necessarily fall into the PartialDependenceError category? Got it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea it's caused by something else in sklearn. Raising here makes sure the final except catches it and then we can wrap as PartialDependenceError with the "all other errors code"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, about the docs page. Yea you have to click on the drop down on the left hand side and then click on exceptions.

The "API Reference" is a manually curated list of functions/classes but it's not exhaustive.

Copy link
Contributor

@ParthivNaresh ParthivNaresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid work, love the Enum implementation

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this change. LGTM!

if any(is_datetime):
raise ValueError(
"Two-way partial dependence is not supported for datetime columns."
X_unknown = X_features.ww.select("unknown")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we combine this with the following check for "URL", "EmailAddress", "NaturalLanguage"? They all have the INVALID_FEATURE_TYPE code :)

@freddyaboulton freddyaboulton force-pushed the 2539-custom-exception-partial-dependence branch from 8650c12 to 2148a52 Compare August 9, 2021 19:06
@freddyaboulton freddyaboulton force-pushed the 2539-custom-exception-partial-dependence branch from 2148a52 to 53b932b Compare August 9, 2021 19:09
@freddyaboulton freddyaboulton merged commit c85233e into main Aug 9, 2021
@freddyaboulton freddyaboulton deleted the 2539-custom-exception-partial-dependence branch August 9, 2021 19:35
@chukarsten chukarsten mentioned this pull request Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise a custom exception for partial dependence errors
5 participants