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

#51 Add option to overlap importance on train and test in plot of ShapModelInterpreter #172

Conversation

eduardo-lourenco
Copy link

This PR targets issue #51:

  • Adds option to have ShapModelInterpreter plots for both train and test sets in the same graph.
  • Updates the tutorial related with model interpretation.

I will update the related tests if this solution seems appropriate (current tests are still passing, we just need to add new tests).

Copy link
Contributor

@Matgrb Matgrb left a comment

Choose a reason for hiding this comment

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

Thanks for the work, i left some comments

plot_type_call = "dot"
plot_title = f"SHAP Summary plot for {target_set_title[i]} set"

if target_set == "both":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add to notebook docs an example how to make this?

@@ -225,22 +233,55 @@ def plot(
raise ValueError("alpha must be a float value betwee 0 and 1")

self.min_q, self.max_q, self.alpha = min_q, max_q, alpha
if other_tdp:
other_tdp.min_q, other_tdp.max_q, other_tdp.alpha = min_q, max_q, alpha
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the other_tdp has lower min_q or higher max_q?
Maybe good to take min and max over quantiles in both plots

self._target_rate_plot(feature=feature, bins=bins, type_binning=type_binning, ax=ax2)

ax2.set_xlim(ax1.get_xlim())
for i, (tdp, (ax1, ax2)) in enumerate(zip([self, other_tdp], ax_input)):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this for loop can be a bit more readable. Do you have some idea how?

@@ -267,7 +308,6 @@ def _dependence_plot(self, feature, ax=None):
ax.scatter(X[y == 1], shap_val[y == 1], label=self.class_names[1], color="darkred", alpha=self.alpha)

ax.set_ylabel("Shap value")
ax.set_title(f"Dependence plot for {feature} feature")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you remove the title?

@Matgrb
Copy link
Contributor

Matgrb commented Jan 23, 2022

@eduardo-lourenco Are you going to still work on this one PR?

@ReinierKoops
Copy link

hi @eduardo-lourenco thanks for your contributions, could you update your PR to be made compatible with the latest changes on main?

@ReinierKoops
Copy link

This PR is closed. Please reach out if you want to continue with this PR.

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.

3 participants