Optim-wip: Add a ton of missing docs #571

ProGamerGov · 2020-12-24T15:49:22Z

No description provided.

ProGamerGov · 2021-05-09T16:21:24Z

The other optim PRs should probably be reviewed before this one.

ProGamerGov · 2021-05-09T19:29:39Z

All the tests should pass once #656 is merged.

* Some minor changes that I forget when I pulled from the optim-wip master branch.

NarineK

Thank you very much for the documentation PR. Overall looks great!
I did a quick pass and left some comments.
I also see that we have Losses that need to be documented a bit more thoroughly .
https://github.com/pytorch/captum/blob/optim-wip/captum/optim/_core/loss.py

In general I think that before open sourcing we can go over docs again, make them a bit more thorough and clean up the formatting, but we can do that in a separate PR as well.

NarineK · 2021-07-25T20:58:20Z

captum/optim/_core/optimization.py

                        every iteration and returns a bool that determines whether
                        to stop the optimization.
                        See captum.optim.typing.StopCriteria for details.
            optimizer (Optimizer, optional):  An torch.optim.Optimizer used to


For the optimize method loss_summarize_fn and lr aren't documented. Do you mind adding documentation their too.

Yeah, I'll add the documentation for those variables!

@ProGamerGov, the documentation for losses we will add in a separate PR ?https://github.com/pytorch/captum/blob/optim-wip/captum/optim/_core/loss.py

@NarineK Yes, we'll add the loss documentation in a separate PR!

NarineK · 2021-07-25T20:59:22Z

captum/optim/_core/output_hook.py

+
+    Args:
+        model (nn.Module):  The reference to PyTorch model instance.
+        targets (nn.module or list of nn.module):  The target layers to


nit: nn.Module

NarineK · 2021-07-25T21:04:00Z

captum/optim/_core/output_hook.py

+            collect activations from.
    """

    def __init__(self, model: nn.Module, targets: Iterable[nn.Module]) -> None:


nit: documentation for init arguments.

I'll move all the init arguments from class descriptions to the class' init functions.

NarineK · 2021-07-25T21:08:56Z

captum/optim/_core/output_hook.py

 class ActivationFetcher:
    """
    Simple module for collecting activations from model targets.
+


Do you mind, please, adding also some docs for ModuleOutputsHook ? Also is, ModuleReuseException used anywhere ?

I don't see ModuleReuseException being called anywhere, so I think that it's safe to remove it.

NarineK · 2021-07-25T21:14:48Z

captum/optim/_param/image/images.py


 class FFTImage(ImageParameterization):
-    """Parameterize an image using inverse real 2D FFT"""
+    """


Do you mind adding some docs for ImageTensor as well ?
Also, are InputParameterization and ImageParameterization used anywhere ?

NarineK · 2021-07-25T22:11:42Z

captum/optim/_param/image/images.py

        return self.image.refine_names("B", "C", "H", "W")


 class LaplacianImage(ImageParameterization):


usually we document all non-underscored methods in the public classes. If we don't want those methods to be public we can underscore them. I see that we have quite some in images.py

I've added some more documentation and added underscore to a bunch of the public class methods!

NarineK · 2021-07-25T22:18:48Z

captum/optim/_param/image/transforms.py


    You can specify a fixed background, or a random one will be used by default.
+
+    Args:


This can go under the __init__.

I've moved it!

NarineK · 2021-07-25T22:21:58Z

captum/optim/_param/image/transforms.py

    def forward(self, x: torch.Tensor) -> torch.Tensor:
+        """
+        Ignore the alpha channel.
+        Arguments:


I think that there is some spacing that we need to do here and for Return.
See: https://github.com/pytorch/captum/blob/master/captum/attr/_core/integrated_gradients.py#L132

I've tried to format all the docs to match the formatting used in the integrated_gradients file you linked to.

NarineK · 2021-07-25T22:25:26Z

captum/optim/_param/image/transforms.py

+        Returns:
+            rgb (torch.Tensor): RGB image tensor without the alpha channel.
+        """
        assert x.dim() == 4


Since klt_transform and i1i2i3_transform are public, it would be good to document them.
__ init__ for ToRGB needs docs as well.

Okay, I've add some documentation and moved the init args from the main description to the init function.

NarineK · 2021-07-25T22:27:24Z

captum/optim/_param/image/transforms.py

+            chw (torch.tensor):  A tensor with it's colors recorrelated or
+                decorrelated.
+        """
+


The __init__ for CenterCrop + RandomScale (public methods), RandomSpatialJitter (public methods) need docs for args as well.

NarineK · 2021-07-31T00:12:09Z

captum/optim/_core/optimization.py

+            loss_summarize_fn (Callable, optional): The function to use for summarizing
+                tensor outputs from loss functions.
+                Default: default_loss_summarize
+            lr: (float): If no optimizer is given, then lr is used as the learning rate


nit: lr: (float, optional)

NarineK

Thank you for addressing the comments. For couple inputs optional was missing otherwise looks great.

NarineK · 2021-07-31T00:14:50Z

captum/optim/_param/image/images.py

+            path (str): A URL or filepath to an image.
+            scale (float): The image scale to use.
+                Default: 255.0
+            mode (str:) The image loading mode to use.


scale (float, optional):
mode (str, optional):

NarineK · 2021-07-31T00:15:15Z

captum/optim/_param/image/images.py

+
+            figsize (Tuple[int, int], optional): height & width to use
+                for displaying the `ImageTensor` figure.
+            scale (float): Value to multiply the `ImageTensor` by so that


NarineK · 2021-07-31T00:15:45Z

captum/optim/_param/image/images.py

+
+            filename (str): The filename to use when saving the `ImageTensor` as an
+                image file.
+            scale (float): Value to multiply the `ImageTensor` by so that


(float, optional)

NarineK · 2021-07-31T00:22:07Z

captum/optim/_param/image/transforms.py

+        """
+        Args:
+
+            multiplier (float):  A float value used to scale the input.


(float, optional)

Add a ton of missing docs

f9ca6c1

facebook-github-bot added the cla signed label Dec 24, 2020

ProGamerGov added 3 commits December 24, 2020 09:07

Add docs for NumPy helpers

ab19efb

Add image docs

395ab63

Add more docs

5a5b285

greentfrapp mentioned this pull request Mar 25, 2021

optim-wip: Fix objectives.py, images.py, RedirectedReLU etc. #552

Merged

ProGamerGov force-pushed the optim-wip branch from c755007 to 5a5b285 Compare May 6, 2021 14:29

ProGamerGov force-pushed the optim-wip branch from cac5172 to 5a5b285 Compare May 9, 2021 16:51

Merge remote-tracking branch 'upstream/optim-wip' into optim-wip

4e65857

ProGamerGov added 4 commits May 9, 2021 15:49

Improve docs

66985f8

Add missing optional to doc

f5a755d

Update docs to reflect new changes

e020ef7

* Some minor changes that I forget when I pulled from the optim-wip master branch.

Merge remote-tracking branch 'upstream/optim-wip' into optim-wip

f67d534

NarineK self-assigned this Jul 25, 2021

NarineK reviewed Jul 25, 2021

View reviewed changes

ProGamerGov added 2 commits July 29, 2021 14:25

Changes based on feedback

7c6ef63

Fix Flake8

1a4c0bf

NarineK reviewed Jul 31, 2021

View reviewed changes

NarineK approved these changes Jul 31, 2021

View reviewed changes

Add missing 'optional's to docs

bdc3f17

NarineK merged commit e7835de into meta-pytorch:optim-wip Jul 31, 2021

		return self.image.refine_names("B", "C", "H", "W")


		class LaplacianImage(ImageParameterization):


		You can specify a fixed background, or a random one will be used by default.

		Args:

Optim-wip: Add a ton of missing docs #571

Optim-wip: Add a ton of missing docs #571

Uh oh!

Conversation

ProGamerGov commented Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProGamerGov commented May 9, 2021

Uh oh!

ProGamerGov commented May 9, 2021

Uh oh!

NarineK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NarineK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ProGamerGov commented Dec 24, 2020 •

edited

Loading