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

Insights module and SHAPInsight #391

Merged
merged 102 commits into from
Jan 20, 2025
Merged

Conversation

Alex6022
Copy link
Contributor

@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!

This was referenced Oct 4, 2024
@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 Alex6022 requested a review from AVHopp January 14, 2025 11:31
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

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

Gave it a very brief additional check, LGTM so here is my approve :)

In case anything comes up where you need an additional opinion, just summon me again, otherwise I have nothing left to say other than thanks for the contribution!

baybe/exceptions.py Show resolved Hide resolved
baybe/insights/shap.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@AdrianSosic AdrianSosic left a comment

Choose a reason for hiding this comment

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

Hi @Alex6022, for my part, the PR is good to go (except for the remaining open threads that I leave to @Scienfitz). Again, many thanks for this great piece of work, and also for your perseverance! 🥇

baybe/insights/shap.py Outdated Show resolved Hide resolved
baybe/insights/shap.py Outdated Show resolved Hide resolved
baybe/insights/shap.py Outdated Show resolved Hide resolved
Include  improvements

Co-authored-by: AdrianSosic <[email protected]>
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.

@Alex6022 many thanks also from my side, super cool contribution, please let us know if you need any reference or endorsement in the future :). This will be part of the next LinkedIn shoutout for the version release, I assume your fine with being explicitly mentioned? If not let me know in any form or way

@AdrianSosic AdrianSosic merged commit 79a94eb into emdgroup:main Jan 20, 2025
8 of 11 checks passed
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.

5 participants