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

Refactor Classwise Shapley #616

Merged
merged 30 commits into from
Aug 21, 2024

Conversation

AnesBenmerzoug
Copy link
Collaborator

Description

This PR refactors the Classwise Shapley valuation to be more aligned with the new Valuation classes.

Changes

  • Implemented Classwise Scorer
  • Implemented Classwise Sampler
  • Implemented Classwise Utility
  • Implemented Classwise Shapley valuation method
  • Added with_idx and with_subset methods to Sample class.
  • Added tests for the new classwise shapley method and a test that compares its output with the old implementation.

Checklist

  • Wrote Unit tests (if necessary)
  • Updated Documentation (if necessary)
  • Updated Changelog
  • If notebooks were added/changed, added boilerplate cells are tagged with "tags": ["hide"] or "tags": ["hide-input"]

@AnesBenmerzoug AnesBenmerzoug changed the base branch from develop to feature/refactor-value August 8, 2024 13:12
@AnesBenmerzoug AnesBenmerzoug marked this pull request as ready for review August 8, 2024 13:12
Copy link
Collaborator

@schroedk schroedk left a comment

Choose a reason for hiding this comment

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

These tests are failing locally on macOS.
Bildschirmfoto 2024-08-12 um 09 47 23

@AnesBenmerzoug
Copy link
Collaborator Author

@schroedk I resolved as many issues as possible but there is still one test that fails for some unknown reason and I am giving up on it. The test in question relies on non-default behaviour, i.e., it does not normalize the values after the valuation. The original paper's implementation normalizes the values and for that case the test passes.

@schroedk
Copy link
Collaborator

@schroedk I resolved as many issues as possible but there is still one test that fails for some unknown reason and I am giving up on it. The test in question relies on non-default behaviour, i.e., it does not normalize the values after the valuation. The original paper's implementation normalizes the values and for that case the test passes.

@AnesBenmerzoug the test is also failing on #558. Let's go for merging this branch and then iterate on #558.
What is missing before we can merge this one?

@AnesBenmerzoug
Copy link
Collaborator Author

@schroedk the test I was talking about is specific to classwise shapley. I have no idea about that other test that is failing.

I think this PR can now be merged.

@schroedk schroedk merged commit 85025c0 into feature/refactor-value Aug 21, 2024
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.

2 participants