Update tutorials to use Layer IG #222
Conversation
NarineK
changed the title
NarineK
changed the title
NarineK
force-pushed
the
update_tutorials_with_layer_ig
branch
from
99f8aef
to
b0487d9
Compare
| if ( | ||
| (hook_inputs is None or len(hook_inputs) == 0) | ||
| and (hook_outputs is None or len(hook_outputs) == 0) | ||
| and len(list((module.parameters()))) == 0 |
There was a problem hiding this comment.
nit: It might be a little cleaner to either move this check into extract_device, since it already does some of these, or just check that returned device is not None and return an error?
| return hook_inputs[0].device | ||
| if hook_outputs is not None and len(hook_outputs) > 0: | ||
| return hook_outputs[0].device | ||
| return next(module.parameters()).device |
There was a problem hiding this comment.
nit: Could add a check for length of module.parameters here, and either return None otherwise or raise the exception here itself?
There was a problem hiding this comment.
I moved the check into this function (haven't pushed the change yet). So, if we have the check on top of this function we don't need to check the length of module.parameters(), do we ? Because otherwise the check will fail ?
(hook_inputs is None or len(hook_inputs) == 0) and (hook_outputs is None or len(hook_outputs) == 0) and len(list((module.parameters()))) == 0
There was a problem hiding this comment.
Actually len(list((module.parameters()))) is going to exhaust all elements - it is not the right way of checking the length - let me fix that
There was a problem hiding this comment.
Makes sense, yeah if you move the check you don't need to check length again! But I guess inputs and output checks are already there, so might be easier to just add the last check to the end, and just raise an exception if none of the return conditions hit as opposed to putting the full check above? But either way works!
There was a problem hiding this comment.
Also doesn't each separate call to module.parameters() return a new iterator?
There was a problem hiding this comment.
- For
module.parameters(), it looks like it is creating a new iterator every time. Good point. But it's better to create it once in our case. - Yeah, I could have the last check on the parameters later but in that case, I'll need to raise new error with a new message and it is probably better to combine the checks in one place.
| if hook_outputs is not None and len(hook_outputs) > 0: | ||
| return hook_outputs[0].device | ||
| return next(module.parameters()).device | ||
|
|
There was a problem hiding this comment.
Might also be helpful to make this a utility method in common to potentially be used by other layer methods? I think layer feature ablation could hit a similar problem, since it checks the device based on inputs, but it's possible inputs is None if attributing with respect to outputs.
| @@ -79,8 +79,6 @@ | |||
| "metadata": {}, | |||
| "outputs": [], | |||
| "source": [ | |||
| "# replace <PATH-TO-SAVED-MODEL> with the real path of the saved model\n", | |||
There was a problem hiding this comment.
nit: did you intend to remove this instruction?
|
Addressed the comments! Thank you! |
NarineK
force-pushed
the
update_tutorials_with_layer_ig
branch
from
6b0c6c5
to
6d1a48e
Compare
There was a problem hiding this comment.
@NarineK has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary:
1. Updated tutorials for IMDB and Bert to use `LayerIntegratedGradients`. In case of Bert it uses
LayerIG only for interpreting parent `BertEmbeddings` layer.
2. Added a fix for extracting devices in case inputs and/or outputs are missing.
Currently it is checking three things: hook inputs, outputs and module.parameters()
3. In the case of Bert I was printing pandas styled DataFrame. In that case, it contained auto-
generated ids for each html element. Now, I formatted it differently so that there is no ID in the
HTML divs etc.
Pull Request resolved: pytorch#222
Differential Revision: D19251959
Pulled By: NarineK
fbshipit-source-id: e28ecb53a7bd5e71cc6c5df210729004da8f3a1e
LayerIntegratedGradients. In case of Bert it usesLayerIG only for interpreting parent
BertEmbeddingslayer.Currently it is checking three things: hook inputs, outputs and module.parameters()
generated ids for each html element. Now, I formatted it differently so that there is no ID in the
HTML divs etc.