Skip to content
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

ConvNormActivation block for 3D convolutions #5430

Closed
jdsgomes opened this issue Feb 16, 2022 · 6 comments · Fixed by #5445
Closed

ConvNormActivation block for 3D convolutions #5430

jdsgomes opened this issue Feb 16, 2022 · 6 comments · Fixed by #5445

Comments

@jdsgomes
Copy link
Contributor

🚀 The feature

I suggest we add a Conv3d equivalent for the ConvNormActivation block

Motivation, pitch

This will be used in the upcoming ResNext3d implementation.

Alternatives

I see two possible alternatives:

  1. We can re-purpose the existing method in a BC way by adding an extra named parameter conv layer with default value of Conv2D.

  2. We can explicitly add a new class Conv3DNormActivation, which will be very similar to the existing one.

Additional context

No response

@oke-aditya
Copy link
Contributor

In my opinion 2nd one is better.

I can give a shot at this :)

@jdsgomes
Copy link
Contributor Author

Thanks for the feedback @oke-aditya!

I was thinking and discussing with @datumbox offline, and I'm inclined to the following solution:

  1. Create an Alias for the existing ConvNormActivation and call it Conv2dNormActivation. (this will be done in a separate PR. I would also add a deprecation warning for the ConvNormActivation in this case.
  2. Create a new method for 3D convolutions named Conv3dNormActivation, which seems to be your preferred method too.

The advantages of this approach is that we have a clear naming convention in line with the nn.Conv2d and nn.Conv3d from pytorch, and we wouldn't have to validate invalid combinations of norm_layer and the new conv_layer paramter in ConvNormActivation.

The major disadvantage is that the current ConvNormActivation is already used in 9 models inside TorchVision and numerous places externally but I think this can be mitigated with the deprecation warning and fazing out in future releases.

@jdsgomes
Copy link
Contributor Author

Also, thank you for offering to give it a shot, let's just wait for some more feedback and then would be great if you wish to work on it!

@datumbox
Copy link
Contributor

The approach sounds good to me. If you bring a separate PR, that does step 1, we should replace all existing uses in TorchVision and also do the same on the next fbsync for all FBcode internal cases.

@jdsgomes
Copy link
Contributor Author

@oke-aditya I kicked off by renaming the ConvNormActivation as we all seem to be aligned. Looking forward to your contribution for step 2.

@oke-aditya
Copy link
Contributor

Nice will do this over the weekend 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants