Some comments about code of PartialConv2d

shaibagon commented 3 years ago

Hi, I really like your work - thank you for sharing the code.

I have some remarks on the code of PartialConv2d:

1. Using buffers instead of simple "member tensors":

The class attribute self.weight_maskUpdater is defined as a "plain" tensor: https://github.com/NVIDIA/partialconv/blob/610d373f35257887d45adae84c86d0ce7ad808ec/models/partialconv2d.py#L32-L35 As a result, when the model is transferred to GPU, or data type is changing you need to explicitly check for it and change it: https://github.com/NVIDIA/partialconv/blob/610d373f35257887d45adae84c86d0ce7ad808ec/models/partialconv2d.py#L49-L50

A more elegant way is to use non-persistent buffers:

        if self.multi_channel:
            self.register_buffer(name='weight_maskUpdater', persistent=False,
                                 tensor=torch.ones(self.out_channels, self.in_channels,
                                                   self.kernel_size[0], self.kernel_size[1]))
        else:
            self.register_buffer(name='weight_maskUpdater', persistent=False,
                                 tensor=torch.ones(1, 1, self.kernel_size[0], self.kernel_size[1]))

This way self.weight_maskUpdater will be affected by any .to(...) / .cuda() / .cpu() invoked on the model hosting this layer, making the condition on line 49 redundent.

2. Use of `torch.ones_like`

Instead of torch.ones(...).to(...) https://github.com/NVIDIA/partialconv/blob/610d373f35257887d45adae84c86d0ce7ad808ec/models/partialconv2d.py#L54-L57 You can use torch.ones_like which is simpler and easier to read:

                    if self.multi_channel:
                        mask = torch.ones_like(input)
                    else:
                        mask = torch.ones_like(input[:1, :1, ...])

3. No need to import `Variable`

You import Variable, but never use it. https://github.com/NVIDIA/partialconv/blob/610d373f35257887d45adae84c86d0ce7ad808ec/models/partialconv2d.py#L12

BTW All these comments are applicable to the code of PartialConv3d.

liuguilin1225 commented 3 years ago

Thanks for your great suggestions. Will update them accordingly after testing.

ivanstepanovftw commented 4 months ago

torch.ones_like(input[:1, :1, ...])

Does input[:1, :1, ...] taking a slice of the tensor without making a copy?

shaibagon commented 4 months ago

@ivanstepanovftw I don't think it makes a copy. However, since it is only used as an argument for torch.ones_like the values of the tensor are not being used at all, only the shape, dtype and device.

ivanstepanovftw commented 3 months ago

Also I think this operation is useless: https://github.com/NVIDIA/partialconv/blob/610d373f35257887d45adae84c86d0ce7ad808ec/models/partialconv2d.py#L75 because input will be multiplied by the mask anyway here: https://github.com/NVIDIA/partialconv/blob/610d373f35257887d45adae84c86d0ce7ad808ec/models/partialconv2d.py#L70

NVIDIA / partialconv