Add type hints to layer_integrated_gradients.py #252

quinton-hoole · 2020-01-24T00:11:54Z

See also: #184

Tests:

captum] black .
reformatted /data/users/quinton/captum/captum/attr/_core/layer/layer_integrated_gradients.py
All done! ✨ 🍰 ✨
1 file reformatted, 105 files left unchanged.
captum] flake8 .
captum] ./scripts/run_mypy.sh
Success: no issues found in 43 source files
Success: no issues found in 48 source files

This is my first contribution to PyTorch, so I want to confirm that my dev process and initial attempt are sound. More methods can be typehinted in this or followup PR's (similar to #184 above).

vivekmig

Thanks for adding this, this looks great! Just a few small changes, and if you could add hints to the remaining helper methods in test_layer_integrated_gradients.py, that would be great!

vivekmig · 2020-01-24T00:37:07Z

captum/attr/_core/layer/layer_integrated_gradients.py

+        n_steps: int = 50,
+        method: str = "gausslegendre",
+        internal_batch_size: Optional[int] = None,
+        return_convergence_delta: Optional[bool] = False,


These two arguments (return_convergence_delta and attribute_to_layer_input) should just have a bool type, since they default to False, not None.

Agreed, done.

vivekmig · 2020-01-24T00:39:45Z

tests/attr/layer/test_layer_integrated_gradients.py

+        model: Module,
+        target_layer: Module,
+        test_input: Union[Tensor, Tuple[Tensor, ...]],
+        expected_ig: Tuple[List[List[float]], List[List[float]]],


nit: This is correct for the existing usage, but to make this more generic, could change this to Tuple[List[List[float]], ...], so we can also pass arbitrary length tuples in the future.

Agreed, done.

vivekmig · 2020-01-28T18:05:49Z

Hi @quinton-hoole , I have made some updates to the class hierarchy for Layer Integrated Gradients here #244, so please merge with these changes when you update this PR, thanks!

quinton-hoole · 2020-01-28T20:59:27Z

@vivekmig Awesome! I was actually a bit stuck on some type hinting apparently related to attributes(). Hopefully this will help to resolve those. Otherwise I will contact you shortly for some help.

quinton-hoole · 2020-01-28T21:08:58Z

@vivekmig Since merging your updates in #244 I'm getting the following error (I had zero errors before the merge). Does not seem to be related to any of the changes I've made. Any advice? I'll push the current state of my branch here for your reference.

./scripts/run_mypy.sh
Success: no issues found in 44 source files
tests/attr/neuron/test_neuron_gradient_shap.py:66: error: "Tensor" has no attribute "__iter__"; maybe "__ior__", "__int__", or "__invert__"? (not iterable)
Found 1 error in 1 file (checked 48 source files)

vivekmig · 2020-01-28T21:14:37Z

@vivekmig Since merging your updates in #244 I'm getting the following error (I had zero errors before the merge). Does not seem to be related to any of the changes I've made. Any advice? I'll push the current state of my branch here for your reference.
./scripts/run_mypy.sh
Success: no issues found in 44 source files
tests/attr/neuron/test_neuron_gradient_shap.py:66: error: "Tensor" has no attribute "__iter__"; maybe "__ior__", "__int__", or "__invert__"? (not iterable)
Found 1 error in 1 file (checked 48 source files)

This error is due to torch 1.3 missing some type hints for tensors, which was fixed here pytorch/pytorch#28578. If you upgrade to torch 1.4, that should fix the issue!

quinton-hoole · 2020-01-28T21:19:49Z

@vivekmig Since merging your updates in #244 I'm getting the following error (I had zero errors before the merge). Does not seem to be related to any of the changes I've made. Any advice? I'll push the current state of my branch here for your reference.
./scripts/run_mypy.sh
Success: no issues found in 44 source files
tests/attr/neuron/test_neuron_gradient_shap.py:66: error: "Tensor" has no attribute "__iter__"; maybe "__ior__", "__int__", or "__invert__"? (not iterable)
Found 1 error in 1 file (checked 48 source files)
This error is due to torch 1.3 missing some type hints for tensors, which was fixed here pytorch/pytorch#28578. If you upgrade to torch 1.4, that should fix the issue!

Awesome! Thanks.

quinton-hoole · 2020-01-28T22:06:21Z

@vivekmig I've pushed what will hopefully be the final commit. mypy tests pass:

  ./scripts/run_mypy.sh
Success: no issues found in 44 source files
Success: no issues found in 48 source files

vivekmig

Thanks for updating this, looks good, just a few small comments! Also, there seem to be some formatting issues causing CircleCI failures, can you run black and flake8?

vivekmig · 2020-01-28T23:36:24Z

captum/attr/_core/layer/layer_integrated_gradients.py

+    def __init__(
+        self,
+        forward_func: Callable,
+        layer: torch.nn.Module,


nit: Can you add "from torch.nn import Module" above and just put Module here? It's consistent with our usage in other files.

Agreed, done.

vivekmig · 2020-01-29T19:43:50Z

captum/attr/_core/layer/layer_integrated_gradients.py

+            forward_fn: Callable,
+            inputs: Union[Tensor, Tuple[Tensor,...]],
+            target_ind: int = None,
+            additional_forward_args: Any = None


Would be great to also type hint the return of this function, it should be Tuple[Tensor, ...]

Agreed, done.

vivekmig · 2020-01-29T20:00:20Z

captum/attr/_core/layer/layer_integrated_gradients.py

        ):
+            scattered_inputs: Union[Tuple[Union[Tensor, Tuple[Tensor, ...]]], Union[Tuple[Tensor, ...], List[Tuple[Any, ...]]]]


I think we can avoid having this extra declaration, and instead just add "# type:ignore" to line 283 below with a short comment above on why this is necessary. This seems to be caused by the scatter method not having a precise enough return type in its stub, so let's just suppress the type warning there.

Agreed, done.

quinton-hoole · 2020-01-30T20:36:23Z

Thanks @vivekmig. I've addressed all the comments and rerun black and flake8.

vivekmig

It seems like run_mypy is throwing an error now, the "type: ignore" needs to be moved to line 285, since the line was reformatted. Also, happened to notice that the targets hint also should be updated, if you can fix that as well and then rerun black, flake8, and run_mypy to double check everything that would be great! Thanks so much for your work on this, these should be the last changes!

vivekmig · 2020-01-31T13:17:04Z

captum/attr/_core/layer/layer_integrated_gradients.py

-        ):
+            forward_fn: Callable,
+            inputs: Union[Tensor, Tuple[Tensor, ...]],
+            target_ind: int = None,


This should actually be the same type as target above, (Optional[Union[int, Tuple[int, ...], Tensor, List[Tuple[int, ...]]]])

Good catch! Thanks, fixed.

quinton-hoole · 2020-01-31T17:05:43Z

It seems like run_mypy is throwing an error now, the "type: ignore" needs to be moved to line 285, since the line was reformatted. Also, happened to notice that the targets hint also should be updated, if you can fix that as well and then rerun black, flake8, and run_mypy to double check everything that would be great! Thanks so much for your work on this, these should be the last changes!

Whoops! My apologies for that oversight. Fixed now.

quinton-hoole · 2020-01-31T18:45:06Z

@vivekmig I think this one is ready to merge now.

vivekmig

Looks great, thanks so much for adding this!

facebook-github-bot

@quinton-hoole has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

NarineK · 2020-02-03T03:42:42Z

captum/attr/_core/layer/layer_integrated_gradients.py

+        method: str = "gausslegendre",
+        internal_batch_size: Optional[int] = None,
+        return_convergence_delta: bool = False,
+        attribute_to_layer_input: bool = False,


Shouldn't this be optional too ?
cc: @vivekmig

I think this is fine the way it is, Optional[X] is equivalent to Union of X and None, in this case attribute_to_layer_input will always take a bool value, even in the default case, and should never be None.

facebook-github-bot · 2020-02-03T21:14:22Z

@quinton-hoole merged this pull request in 6686d67.

Summary: See also: pytorch#184 Tests: captum] black . reformatted /data/users/quinton/captum/captum/attr/_core/layer/layer_integrated_gradients.py All done! ✨ 🍰 ✨ 1 file reformatted, 105 files left unchanged. captum] flake8 . captum] ./scripts/run_mypy.sh Success: no issues found in 43 source files Success: no issues found in 48 source files This is my first contribution to PyTorch, so I want to confirm that my dev process and initial attempt are sound. More methods can be typehinted in this or followup PR's (similar to pytorch#184 above). Pull Request resolved: pytorch#252 Reviewed By: vivekmig Differential Revision: D19669052 Pulled By: quinton-hoole fbshipit-source-id: e74830a6c4528a8f60b5358dc9c0cda03edc85ae

vivekmig reviewed Jan 24, 2020

View reviewed changes

quinton-hoole added 2 commits January 28, 2020 13:05

Add type hints to layer_integrated_gradients.py

ea15f45

Address some review comments

2ca4cef

quinton-hoole force-pushed the 2020-01-23-type-hint-layer_integrated_gradients branch from 5c1a65d to 2ca4cef Compare January 28, 2020 21:10

Addressed remaining comments

df9c209

vivekmig reviewed Jan 29, 2020

View reviewed changes

Addressed final review comments

a22b990

vivekmig reviewed Jan 31, 2020

View reviewed changes

Fixed type hint on gradient_func.target_ind, and reformatted correctly

73db8b6

vivekmig approved these changes Jan 31, 2020

View reviewed changes

facebook-github-bot reviewed Jan 31, 2020

View reviewed changes

NarineK reviewed Feb 3, 2020

View reviewed changes

facebook-github-bot closed this in 6686d67 Feb 3, 2020

facebook-github-bot added the Merged label Feb 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints to layer_integrated_gradients.py #252

Add type hints to layer_integrated_gradients.py #252

quinton-hoole commented Jan 24, 2020

vivekmig left a comment

vivekmig Jan 24, 2020

quinton-hoole Jan 30, 2020

vivekmig Jan 24, 2020

quinton-hoole Jan 30, 2020

vivekmig commented Jan 28, 2020

quinton-hoole commented Jan 28, 2020

quinton-hoole commented Jan 28, 2020 •

edited

vivekmig commented Jan 28, 2020

quinton-hoole commented Jan 28, 2020

quinton-hoole commented Jan 28, 2020

vivekmig left a comment

vivekmig Jan 28, 2020

quinton-hoole Jan 30, 2020

vivekmig Jan 29, 2020

quinton-hoole Jan 30, 2020

vivekmig Jan 29, 2020

quinton-hoole Jan 30, 2020

quinton-hoole commented Jan 30, 2020

vivekmig left a comment

vivekmig Jan 31, 2020

quinton-hoole Jan 31, 2020

quinton-hoole commented Jan 31, 2020

quinton-hoole commented Jan 31, 2020

vivekmig left a comment

facebook-github-bot left a comment

NarineK Feb 3, 2020

vivekmig Feb 3, 2020

facebook-github-bot commented Feb 3, 2020

		):
		scattered_inputs: Union[Tuple[Union[Tensor, Tuple[Tensor, ...]]], Union[Tuple[Tensor, ...], List[Tuple[Any, ...]]]]

Add type hints to layer_integrated_gradients.py #252

Add type hints to layer_integrated_gradients.py #252

Conversation

quinton-hoole commented Jan 24, 2020

vivekmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivekmig commented Jan 28, 2020

quinton-hoole commented Jan 28, 2020

quinton-hoole commented Jan 28, 2020 • edited

vivekmig commented Jan 28, 2020

quinton-hoole commented Jan 28, 2020

quinton-hoole commented Jan 28, 2020

vivekmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinton-hoole commented Jan 30, 2020

vivekmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinton-hoole commented Jan 31, 2020

quinton-hoole commented Jan 31, 2020

vivekmig left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 3, 2020

quinton-hoole commented Jan 28, 2020 •

edited