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

Fix bug in explain predictions due to mixed int string feature names #1871

Merged

Conversation

freddyaboulton
Copy link
Contributor

Pull Request Description

Fixes #1812


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 Feb 22, 2021

Codecov Report

Merging #1871 (55f2be7) into main (eb3bf8f) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1871   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         266      266           
  Lines       21348    21348           
=======================================
  Hits        21342    21342           
  Misses          6        6           
Impacted Files Coverage Δ
...s/prediction_explanations_tests/test_algorithms.py 100.0% <ø> (ø)
...derstanding/prediction_explanations/_algorithms.py 97.2% <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 eb3bf8f...55f2be7. Read the comment docs.

@freddyaboulton freddyaboulton force-pushed the 1812-explain-predictions-mixed-int-str-feature-names branch from ff2a315 to bea4c18 Compare February 25, 2021 17:33
@freddyaboulton freddyaboulton changed the title 1812 explain predictions mixed int str feature names Fix bug in explain predictions due to mixed int string feature names Feb 25, 2021
@freddyaboulton freddyaboulton force-pushed the 1812-explain-predictions-mixed-int-str-feature-names branch from bea4c18 to 401d022 Compare February 25, 2021 22:21
@freddyaboulton freddyaboulton marked this pull request as ready for review February 25, 2021 22:59
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! 🥳

@freddyaboulton I wanted to run the repro in the issue but couldn't, maybe because some of our apis have changed. Still, the impl makes sense and I see the mixed int/str cols in the tests.

(I also just realized you never requested review I vaguely remember you mentioning this during standup a few days ago so my bad if this wasn't ready yet 😂)

Copy link
Collaborator

@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 really love these kinds of PRs

@freddyaboulton freddyaboulton force-pushed the 1812-explain-predictions-mixed-int-str-feature-names branch from 401d022 to 55f2be7 Compare March 1, 2021 14:39
@freddyaboulton freddyaboulton merged commit b261329 into main Mar 1, 2021
@freddyaboulton freddyaboulton deleted the 1812-explain-predictions-mixed-int-str-feature-names branch March 1, 2021 15:28
@dsherry dsherry mentioned this pull request Mar 11, 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.

Explain predictions functions don't work if features have mixed int/string names
3 participants