add option for mean and std to be tuples

[JoanFM]

Description

I propose to allow the user to instantiate Normalize object with mean and std as tuples.

Status

Ready

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or new feature that would cause existing functionality to change)
• New tests added to cover the changes
• Docstrings/Documentation updated

[edgarriba]

add this extra test

[shijianjian]

Please remove those prints first.

In principle, I think it is better to keep all functional functions in a tensor only setting, with less tensor initializations. The tuple might be provided as the syntax sugar for nn.Modules, in which tensors might be initialised and stored properly in the module.

[JoanFM]

Please remove those prints first.

In principle, I think it is better to keep all functional functions in a tensor only setting, with less tensor initializations. The tuple might be provided as the syntax sugar for nn.Modules, in which tensors might be initialised and stored properly in the module.

Sure, prints are leftover, because I am not sure how, but my test was not going to the function I expected. And I still not understand.

I agree functions can work with torch.Tensors but initialization should support regular python tuples, I think it feels more natural to provide mean or std as regular python objects than with torch.Tensor, why to put such burden in the user end?

But yes, adding it as syntax sugar in the nn module discussed in the issue #520 would work for me

[JoanFM]

Hey @shijianjian .

What I think it would make sense is to review everywhere in the code and allow only to provide mean and std as torch.Tensor at this stage.

So the conversion from whatever accepted input to torch.Tensor is in every instance constructor.

Like this you can skip all these checks and conversions at every functional iteration, and keep the code more mantainable

[JoanFM]

Actually, I am not sure, are these functions that the user should be able to use from their own code (are they public) or are they to only be used by the internal kornia objects (therefore private)?

If so, I think this strategy should be followed in general and code will be better unittested maybe?

[shijianjian]

Hey @shijianjian .

What I think it would make sense is to review everywhere in the code and allow only to provide mean and std as torch.Tensor at this stage.

So the conversion from whatever accepted input to torch.Tensor is in every instance constructor.

Like this you can skip all these checks and conversions at every functional iteration, and keep the code more mantainable

Correct. That is exactly what I meant. You only think about tensors for functional calls, and provide some easy API with nn.Module.

[shijianjian]

I agree functions can work with torch.Tensors but initialization should support regular python tuples, I think it feels more natural to provide mean or std as regular python objects than with torch.Tensor, why to put such burden in the user end?

I don't think it is a good design to convert a tuple or list or a number to torch tensor in a functional call. It firstly reduced the efficiency whilst init and destroy small tensors. I think I have discussed with @edgarriba at some point about this, that to keep the Kornia core functions to accept purely tensors.

But yes, adding it as syntax sugar in the nn module discussed in the issue #520 would work for me

The syntax sugar I meant is to support different data types like tuple and lists in modulized implementations. In which way the mean and std will only be generated at the initialisation stage and will be reused afterwards.

In short, functional functions support tensor only, while the nn.Module implementation is the more user friendly choice.

[JoanFM]

I agree functions can work with torch.Tensors but initialization should support regular python tuples, I think it feels more natural to provide mean or std as regular python objects than with torch.Tensor, why to put such burden in the user end?

I don't think it is a good design to convert a tuple or list or a number to torch tensor in a functional call. It firstly reduced the efficiency whilst init and destroy small tensors. I think I have discussed with @edgarriba at some point about this, that to keep the Kornia core functions to accept purely tensors.

But yes, adding it as syntax sugar in the nn module discussed in the issue #520 would work for me

The syntax sugar I meant is to support different data types like tuple and lists in modulized implementations. In which way the mean and std will only be generated at the initialisation stage and will be reused afterwards.

In short, functional functions support tensor only, while the nn.Module implementation is the more user friendly choice.

Sorry, I missunderstood.

I totallly agree, would it be good if I do the conversion at init level? even for float std and mean? That would involve adding an optional parameter num_channels in case your std or mean are floats.

Would that make sense? I think is an effort that makes sense to avoid anything not tensors in the functional calls.

[shijianjian]

I totallly agree, would it be good if I do the conversion at init level? even for float std and mean? That would involve adding an optional parameter num_channels in case your std or mean are floats.

Yes. All tensors shall be prepared in init. I would discard float option.

Would that make sense? I think is an effort that makes sense to avoid anything not tensors in the functional calls.

Yes. Much better.

[edgarriba]

@JoanFM check the mypy - something went wrong

[edgarriba]

@JoanFM you just need to rebase from master to fix the latest pytorch 1.9 formatting issues