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

Don't allow certain logical types in partial_dependence #2573

Merged
merged 14 commits into from
Aug 2, 2021

Conversation

bchen1116
Copy link
Contributor

fix #2547

@bchen1116 bchen1116 self-assigned this Jul 29, 2021
@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #2573 (b12a25f) into main (a741641) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2573     +/-   ##
=======================================
+ Coverage   99.9%   99.9%   +0.1%     
=======================================
  Files        291     291             
  Lines      26644   26664     +20     
=======================================
+ Hits       26601   26621     +20     
  Misses        43      43             
Impacted Files Coverage Δ
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 a741641...b12a25f. Read the comment docs.

@bchen1116 bchen1116 requested a review from a team July 29, 2021 22:07
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.

I think this looks good. The test needs to be changed to actually parameterize over the different disallowed types, though, I think.

Copy link
Contributor

@eccabay eccabay left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me! Just a few places where I think you can clean up the logic a bit to make it more readable.

evalml/model_understanding/graphs.py Outdated Show resolved Hide resolved
evalml/model_understanding/graphs.py Outdated Show resolved Hide resolved
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.

LGTM! Thanks for pulling that copy pasta.

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.

LGTM! Left some nitpicky comments but nothing blocking 😁

evalml/model_understanding/graphs.py Show resolved Hide resolved
@chukarsten chukarsten merged commit 9294f8c into main Aug 2, 2021
@chukarsten chukarsten mentioned this pull request Aug 3, 2021
@freddyaboulton freddyaboulton deleted the bc_2547_partial branch May 13, 2022 15:02
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.

Do not Allow Partial Dependence for NaturalLanguage, URL, EmailAddress
4 participants