-
Notifications
You must be signed in to change notification settings - Fork 334
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
(Keras images) Add an optional image argument, and other improvements #329
Conversation
Codecov Report
@@ Coverage Diff @@
## master #329 +/- ##
==========================================
+ Coverage 97.29% 97.31% +0.02%
==========================================
Files 49 49
Lines 3107 3134 +27
Branches 581 584 +3
==========================================
+ Hits 3023 3050 +27
Misses 44 44
Partials 40 40
|
0503324
to
c4eb039
Compare
@teabolt I'm not sure I follow the comments in previous PR regarding this. Could you please summarize, why should image argument be required, not optional? |
@kmike making
Edit: I can see how my arguments are edge cases. We can try infer things from |
Interesting - in most other places we're dispatching based on model, not based on input data type. E.g. you've got ResNet, with 2d convolutions and stuff => dispatch to image handling.
Yeah, that's what I was thinking - don't require user to pass image if we can figure it out automatically; if it works in 80% cases that's fine, as soon as there is a way to pass image explicitly. |
That is a good idea. For text we can check for Embedding, recurrent, or 1D layers. For images we'd expected 2D layers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @teabolt , looks great 👍
Left some minor comments above
README.rst
Outdated
@@ -51,6 +51,9 @@ It provides support for the following machine learning frameworks and packages: | |||
* sklearn-crfsuite_. ELI5 allows to check weights of sklearn_crfsuite.CRF | |||
models. | |||
|
|||
* Keras_ - explain predictions of image classifiers via Grad-CAM visualizations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it look more 😎 we may move Keras upper the list, e.g. right after scikit-learn (both here and in overview); it can be easy to miss otherwise, as with lightning / sklearn-crfsuite the list starts to be a bit obscure.
Co-Authored-By: Konstantin Lopuhin <[email protected]>
Co-Authored-By: Konstantin Lopuhin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @teabolt 👍
tests/test_keras_integration.py
Outdated
|
||
def test_explain_prediction_not_supported(): | ||
res = eli5.explain_prediction(Sequential(), np.zeros((0,))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this check for "image task / non-image task" faling only because of doc shape, or because of model as well? If that's only doc, what do you think about adding a case where it fails because of a model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, here it fails because of doc
and short-circuits. Added another test case.
Added a Grad-CAM image (https://github.com/teabolt/eli5/blob/keras-gradcam-img-v2/docs/source/static/gradcam-catdog.png) to the README after text highlighting image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-Authored-By: Mikhail Korobov <[email protected]>
Thanks @kmike. I'll have to clean up my backslashes in other PR's. |
Thanks @teabolt! |
API improvements for #315.
For Keras image classifiers, the call for explanations is
eli5.explain_prediction(model, doc)
. We will add the ability to pass an optionalimage
argument like this:eli5.explain_prediction(model, doc, image=image)
, whereimage
is a Pillow image corresponding todoc
. In this case we will no longer usedoc
to create an image. This is useful for experimental models or unusual input shapes.These changes should've been introduced in #325 but need to be merged earlier. See also #315 (comment). (Originally
image
was going to be a required argument, but we decided to keep it optional).