Rename ConvNormActivation to Conv2dNormActivation

[jdsgomes]

Addresses #5430

Renames ConvNormActivation to Conv2dNormActivation and adds deprecation warning in ConvNormActivation

cc @datumbox @oke-aditya

[facebook-github-bot]

💊 CI failures summary and remediations

As of commit 79f6c6d (more details on the Dr. CI page):

---

• 1/1 failures introduced in this PR

---

1 failure not recognized by patterns:

Job	Step	Action
lint_python_and_config	Lint Python code and config files	🔁 rerun

---

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[datumbox]

1 more new line needed.

[datumbox]

Suggested change

	"Use torchvision.ops.misc.ConvNormActivation instead.",
	"Use torchvision.ops.misc.Conv2dNormActivation instead.",

[oke-aditya]

Can we use layers on line 105 as nn.ModuleList ? I think that's what pytorch recommends for storing list of layers.

[oke-aditya]

Also smallest nit. There is a typo in convolution spelling on Line 79.
By the way where do we test this function?

[datumbox]

@oke-aditya Thanks for the comments, I wish Github could allow comments on unmodified lines. It would assist reviews so much.

Can we use layers on line 105 as nn.ModuleList ?

I think it's fine to extend directly from Sequential here. It's an idiom we use often at TorchVision.

Also smallest nit. There is a typo in convolution spelling on Line 79.

Great spot! torchvision/ops/misc.py line 79 reads convolutiuon

By the way where do we test this function?

We test it as part of multiple models that use it. There is a point to be made that each op could be tested separately, but this is quite common with many layers (for example BackboneWithFPN).

[jdsgomes]

Thank you @datumbox and @oke-aditya for the comments. I will address those.

But before that, and taking into account the discussion in #5445, I am thinking if I should instead reformulate this as making ConvNormActivation parametrisable with the type of convolution.

Then both Conv2dNormActivation and Conv3dNormActivation would inherit from the parent ConvNormActivation.

In this version we would have the disadvantage of making ConvNormActivation more complicated as discussed earlier but we wouldn't have to deprecate it, and the inheritance would be more natural. Thoughts?

[datumbox]

@jdsgomes Sounds good to me. I wouldn't worry too much about trying to detect all the possible incorrect combos that the user might pass to the base class in that case. I think ConvNormActivation can be extended in a BC way and then use the 2d and 3d versions as shortcuts.

Let's see how the implementation looks like and chat again.

[oke-aditya]

Yeah we can do this. Also just name the base class as _convrNormActivation. As I think that is not supposed to be used. And we can then deprecate ConvNormActivation.

[oke-aditya]

Looks like me and @datumbox just jinxed at same time to comment. 👀 I can try doing this, but will take a couple of days time.

[jdsgomes]

@oke-aditya the timeline works, so fell free to give it a shot.

I will cancel this PR and suggest a new PRs:

• turn convNormActivation into _convNormActivation and make it generic, plus create an alias with a deprecation warning.
• create the 2d and 3d versions of _convNormActivation

Finally, if your priorities change just let me know and I can work on it.

[oke-aditya]

I will give this a go in couple of days. 😄