-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Fixing one of the captum
tests.
#9549
base: master
Are you sure you want to change the base?
Conversation
By my comment in #9513, I meant why does our CI is still green even without this fix? I am a bit hesitant to merge this without further understanding. |
Our CI is green only because the 187 tests from the unit tests require the |
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9549 +/- ##
==========================================
+ Coverage 86.96% 87.38% +0.41%
==========================================
Files 468 482 +14
Lines 30918 31396 +478
==========================================
+ Hits 26887 27434 +547
+ Misses 4031 3962 -69 ☔ View full report in Codecov by Sentry. |
@akihironitta @rusty1s anything else needed to merge? |
The current PR provides the fix for the following 9 failing tests.
NOTE: This is a resubmission of PR#9513, which had some issues with my
captum
branch.Additional Details Regarding the Fix...
If
method == 'ShapleyValueSampling' and type(index) != int
we do have a situation when tensoreval_diff
which is calculated here has a size of 3. It is impossible to reshape it toshape = [-1, 2, 1, 1]
which is calculated here.My fix eliminates this specific combination of input parameters from being considered in the
test_captum_explainer_multiclass_classification
test.