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

Tutorial notebook for KernelSHAP for images. #126

Merged
merged 13 commits into from
Jan 26, 2022

Conversation

geek-yang
Copy link
Member

@geek-yang geek-yang commented Jan 20, 2022

Two notebooks are added/revised to showcase the KernelSHAP implementation in DIANNA:

  • Explain binary MNIST using KernelSHAP
  • Explain geometric shapes with KernelSHAP

These two notebooks are generated based on the old tutorial notebooks in DIANNA-exploration and DIANNA repo. Major changes include:

  • Use high-level DIANNA API (instead of calling the method directly from dianna.methods)
  • Add more explanation and remove unnecessary (testing/investigation) scripts

Close #95

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@geek-yang
Copy link
Member Author

The explanation of model used to classify geometric shapes is hard to interpret, even after quite some finetuning of parameters. This is an known issue during the exploration stage when we tried to explain it with deeplifthshap using the captum implementation. I would suggest we keep it for now and make another issue for later.

@geek-yang geek-yang marked this pull request as ready for review January 25, 2022 14:30
@geek-yang
Copy link
Member Author

Still cannot fix the import error in the test of notebook ImportError: IProgress not found. Please update jupyter and ipywidgets. See https://ipywidgets.readthedocs.io/en/stable/user_install.html. Any idea about it?

@loostrum
Copy link
Member

I saw that error too, not sure what to do about it apart from trying the suggestions at the suggested URL. I haven't run into it on my laptop. Perhaps you can make an issue for it, if there isn't one already?

The explanation of model used to classify geometric shapes is hard to interpret, even after quite some finetuning of parameters. This is an known issue during the exploration stage when we tried to explain it with deeplifthshap using the captum implementation. I would suggest we keep it for now and make another issue for later.

I agree, let's make an issue and look into it later.

Is there a specific reason there are 2 notebooks for this method? I though we'd do one notebook for each modality/method combination. It would be a shame to throw away work put in the second notebook, so if we decide to keep one here, perhaps we could still store the other one in dianna-exploration.

@loostrum
Copy link
Member

Patrick fixed the ipywidgets issue in another PR (#111), so we should be good on that front.

@geek-yang
Copy link
Member Author

geek-yang commented Jan 25, 2022

I saw that error too, not sure what to do about it apart from trying the suggestions at the suggested URL. I haven't run into it on my laptop. Perhaps you can make an issue for it, if there isn't one already?

Sounds good. I will make one issue.

The explanation of model used to classify geometric shapes is hard to interpret, even after quite some finetuning of parameters. This is an known issue during the exploration stage when we tried to explain it with deeplifthshap using the captum implementation. I would suggest we keep it for now and make another issue for later.

I agree, let's make an issue and look into it later.

Create issue #160 for it.

Is there a specific reason there are 2 notebooks for this method? I though we'd do one notebook for each modality/method combination. It would be a shame to throw away work put in the second notebook, so if we decide to keep one here, perhaps we could still store the other one in dianna-exploration.

Thanks for your comment @loostrum . During the planning meeting we mentioned that for each tutorial notebook for images, we prefer to have one notebook per method unless the one that we already have is only MNIST. For kernelshap I started with MNIST (and the results look good). Given that currently no method makes use of geometric shapes, I also add one notebook for it. That's why. But I think it is ok to put them in the tutorial folder, although it is not necessary to mention them all in the documentation now. For later, the more the better, I guess :)

@egpbos
Copy link
Member

egpbos commented Jan 25, 2022

Indeed, that one is fixed there. The fix was to just install ipywidgets :) So ece8305 can probably be reverted (if there's nothing else in there).

Edit: I see now you actually added ipywidgets to the dependencies as well :) All the other stuff mentioned in the link seems unnecessary though, so that can be taken out of the notebook again.

@geek-yang
Copy link
Member Author

Indeed, that one is fixed there. The fix was to just install ipywidgets :) So ece8305 can probably be reverted (if there's nothing else in there).

Edit: I see now you actually added ipywidgets to the dependencies as well :) All the other stuff mentioned in the link seems unnecessary though, so that can be taken out of the notebook again.

Thanks @egpbos for handling this :).

@egpbos egpbos self-requested a review January 25, 2022 16:06
Copy link
Member

@egpbos egpbos left a comment

Choose a reason for hiding this comment

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

LGTM! Tests pass on my laptop like this. Let's wait for CI and merge if everything is ok.

@egpbos
Copy link
Member

egpbos commented Jan 25, 2022

The dead link is not caused by this issue, so that's fine.

Copy link
Contributor

  • I propose to try another image with a triangle. That might end up being more interpretable (because it has sides and corners).
  • Wouldn't it be better to use the blue (not relevant) <----> red (relevant) colormap for our heatmaps? (In all notebooks.)

@egpbos
Copy link
Member

egpbos commented Jan 25, 2022

@elboyran do you think those things need to be changed in this PR or should we create a new issue for them and merge now?

@elboyran
Copy link
Contributor

For now

@elboyran do you think those things need to be changed in this PR or should we create a new issue for them and merge now?

For now we can merge, but there are conflicts to resolve first.

@geek-yang
Copy link
Member Author

For now

@elboyran do you think those things need to be changed in this PR or should we create a new issue for them and merge now?

For now we can merge, but there are conflicts to resolve first.

The conflicts are resolved. I will merge this branch. @elboyran I will add your comments into the existing issue #160 and get back to it later. Thanks for the suggestions!

@geek-yang geek-yang merged commit 3e87de9 into main Jan 26, 2022
@geek-yang geek-yang deleted the 95-notebook-kernelshap-images branch January 26, 2022 08:55
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.

Tutorial notebook for KernalSHAP for images.
4 participants