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 on force plots to show feature values for specific rows in training data #3044

Merged
merged 16 commits into from Dec 1, 2021

Conversation

peterataylor
Copy link
Contributor

@peterataylor peterataylor commented Nov 15, 2021

…Fixes bug where only first row feature values are show in all row plots.

Pull Request Description

Fixes bug as described in #3045.


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.

…Fixes bug where only first row feature values are show in all row plots.
@CLAassistant
Copy link

CLAassistant commented Nov 15, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #3044 (917f078) into main (2598cfa) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3044     +/-   ##
=======================================
+ Coverage   99.8%   99.8%   +0.1%     
=======================================
  Files        313     313             
  Lines      30567   30579     +12     
=======================================
+ Hits       30477   30489     +12     
  Misses        90      90             
Impacted Files Coverage Δ
evalml/model_understanding/force_plots.py 100.0% <100.0%> (ø)
.../prediction_explanations_tests/test_force_plots.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 2598cfa...917f078. Read the comment docs.

@freddyaboulton
Copy link
Contributor

@peterataylor Do you need help getting all the checks to pass?

@peterataylor
Copy link
Contributor Author

peterataylor commented Nov 16, 2021

@peterataylor Do you need help getting all the checks to pass?

Thanks for the offer @freddyaboulton ! Having a quick look I should be able to do the release notes. But any guidance on the lint, code coverage and dependency changes would be great. But I will look over the contrib guide again and check the approach.

@freddyaboulton
Copy link
Contributor

@peterataylor The "Detect dependency changes" should be fixed as soon as you merge in the latest main branch.

If you have the development dependencies installed, you should be able to do the following from the top level of the repo:

make lint-fix
make lint

If make lint still fails, the errors should hopefully be clear.

If you do not have make installed you can look at the Makefile to see what those commands mean and run then yourself.

Let me know if you have any other questions!

@peterataylor peterataylor marked this pull request as ready for review November 18, 2021 04:48
@@ -45,12 +40,13 @@ def gen_force_plot(shap_values, training_data, expected_value, matplotlib):
initjs()

shap_plots = force_plot(pipeline, rows_to_explain, training_data, y)
for row in shap_plots:
for ix, row in enumerate(shap_plots):
Copy link
Contributor

Choose a reason for hiding this comment

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

@peterataylor This looks good to me! Thanks for the contribution. I think the one thing that's missing is maybe a unit test to make sure we don't commit the same mistake again in the future. Maybe do one with and without a custom index on training_data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @freddyaboulton . I've added some tests to test_force_plots that use the existing parameter sets (rows 0 and 0-4). It required inspection of the returned plot object to ensure the feature values matched those that correspond to the rows_to_explain. I confirmed the tests failed with previous version and pass now. You will probably want to review though. Thanks!

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

@peterataylor This looks good to me! Thank you for the contribution.

@freddyaboulton freddyaboulton merged commit 840fc3b into alteryx:main Dec 1, 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.

None yet

4 participants