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

Feature/shap utils #391

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from
Draft

Conversation

Alex6022
Copy link

@Alex6022 Alex6022 commented Oct 4, 2024

Dear @Scienfitz and @AdrianSosic,

As you previously offered in PR #335, I have taken a shot at integrating SHAP analysis amongst other explainers provided by the SHAP package. Similar to the Polars integration, it is provided as an optional dependency. As suggested, I have included tests, ensured it works with hybrid spaces and molecular encodings, implemented it as another utility and decoupled the computation and plotting methods. It also uses the exposed surrogate implemented in PR #355.

The Explanation object provided by the SHAP package can be created through
shap_kern = explanation(campaign)

This object can then be passed to the plotting functions wrapped from the SHAP package:
plot_beeswarm(shap_kern)

Additionally, this implementation allows users to selected whether to use the computational or experimental representation of the search space. E.g., while the experimental representation can give a good overview of the important parameters:
image

, the computational representation can give more advanced users the option to understand which encodings specifically predict the target:
image

Please let me know if you have any further input for improving this. Looking forward to it!

@AdrianSosic
Copy link
Collaborator

Hi @Alex6022, awesome that you gave it a shot 🎖️ I just returned from my vacation on the weekend. Let me have a thorough look at the code, exchange with @Scienfitz, and then we'll share with you our consolidated thoughts 👍🏼

@AdrianSosic AdrianSosic mentioned this pull request Oct 7, 2024
Copy link
Collaborator

@Scienfitz Scienfitz left a comment

Choose a reason for hiding this comment

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

sorry for taking long, busy days :) but we see your contribution 👍
I'm leaving some initial comments here (and theres also something for you in #357), as I took a first look now, I'll discuss the structure question with the others, some more requests especially regarding the main file will definitely come later

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pytest.ini Outdated Show resolved Hide resolved
baybe/utils/diagnostics.py Outdated Show resolved Hide resolved
…optional dependencies, diagnostics subpackage.
@Alex6022
Copy link
Author

Dear @Scienfitz,
Thank you for the feedback. I have improved the code according to each input from you in this PR and also generalized the code for other SHAP explainers according to your suggestion in #357. Please let me know when and how I should adjust the main functionality according to the overall strategy for this feature :).

@Scienfitz
Copy link
Collaborator

Scienfitz commented Nov 22, 2024

Thank you @Alex6022
fantastic that it could be generalized
In the meantime we came to a conclusion about the structure. I've summarized everything at the top of #357 as tick-off box. Please let me know if you have any questions in the thread. I hope this should mostly only require renamings and moving your already built code around

after that I dont really see any more major changes for now and we should be ready for the full review and finish line 🎉

@Scienfitz Scienfitz marked this pull request as draft November 25, 2024 13:06
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