Skip to content

KernelShap Refactoring #207

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 39 commits into from
May 20, 2020
Merged

KernelShap Refactoring #207

merged 39 commits into from
May 20, 2020

Conversation

alexcoca
Copy link
Contributor

@alexcoca alexcoca commented Apr 13, 2020 •
edited
Loading

Summary of changes:

  • rank_by_importance is no longer a method of the KernelShap object. It is a standalone function which takes a list of shap values as input and, optionally, a list of feature names
  • updated test_rank_by_importance function accordingly
  • updated types to feature_names and categorical_names to KernelShap constructor
  • updated api defaults

@alexcoca alexcoca requested a review from jklaise April 13, 2020 12:34
@alexcoca alexcoca marked this pull request as draft April 16, 2020 15:16
@codecov
Copy link

codecov bot commented Apr 16, 2020 •
edited
Loading

Codecov Report

Merging #207 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #207   +/-   ##
=======================================
  Coverage   85.88%   85.88%           
=======================================
  Files          49       49           
  Lines        5470     5470           
=======================================
  Hits         4698     4698           
  Misses        772      772

@alexcoca alexcoca force-pushed the KernelShapChanges branch from 042c31b to e385bba Compare April 21, 2020 12:15
jklaise
jklaise suggested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@jklaise jklaise left a comment

Choose a reason for hiding this comment

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

Thanks Alex, looks good to me. Just a few small points, mostly clarifying the use/scope/assumptions of the revamped sum_categories function.

jklaise
jklaise reviewed May 7, 2020
View reviewed changes
jklaise
jklaise reviewed May 7, 2020
View reviewed changes
Copy link
Contributor

@jklaise jklaise left a comment

Choose a reason for hiding this comment

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

Just a note, the 2 binary tree files introduced in this commit be removed from this PR as they presumably relate to the Tree shap PR.

@alexcoca
Copy link
Contributor Author

Just a note, the 2 binary tree files introduced in this commit be removed from this PR as they presumably relate to the Tree shap PR.

Removed!

@alexcoca alexcoca marked this pull request as ready for review May 15, 2020 12:20
@alexcoca alexcoca marked this pull request as draft May 15, 2020 12:20
@jklaise jklaise marked this pull request as ready for review May 20, 2020 16:08
@alexcoca alexcoca marked this pull request as ready for review May 20, 2020 16:09
@jklaise jklaise merged commit 7d73473 into SeldonIO:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklaise jklaise jklaise approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexcoca @jklaise