SciNim / flambeau

Nim bindings to libtorch
85 stars 3 forks source link

[RFC] Should we keep dimensions (like Arraymancer) or squezze dimensions (like Pytorch) by default #11

Closed HugoGranstrom closed 1 year ago

HugoGranstrom commented 3 years ago

When we for example slice a Tensor or perform a reduction along an axis we have two options: either to keep the dimensions of the original Tensor even if its size along that axis is 1. Or we can squeeze away that extra dimension. For example, let's say we have this Tensor:

1 2
3 4
Tensor[2, 2]

If we slice it like this: t[_, 0] should we get back a tensor like

1
3
Tensor[2, 1] # Arraymancer

or

1 3
Tensor[2] # Pytorch

I vouch for going against libtorch's defaults and use the Arraymancer way. The reason for this is that it is trivial to squeeze away the unnecessary dimensions with t.squezze while it is non-trivial to restore that extra dimension(s).

mratsim commented 3 years ago

Note that Arraymancer provides at to have dimension coalescing in a less verbose way. t.at(_, 0)

LibTorch provides keepdim parameter for every native LibTorch function which default to false. The [] in flambeau is maps to keepdim = false.

Personally I think it's best if we follow PyTorch/Torch to ease porting existing programs by hopefully copy-paste.

HugoGranstrom commented 3 years ago

That's a fair point! 👍 But it wouldn't hurt to have a similar version here but the other way around? So that t.at() sets keepDim=true?

Clonkk commented 3 years ago

I agree. Changing Torch's convention will break expectation and make the bindings "less intuitive" when compared to existing Torch code.

HugoGranstrom commented 3 years ago

My mind is already in the high-level wrapper it seems 🙃

elliotwaite commented 3 years ago

I think for Flambeau, having it match PyTorch's behavior would be better for portability of code and to make the transition easier for users coming from PyTorch. So to squeeze the dimension: t[_, 0], and to keep the dimension: t[_, 0..0]. I also find this intuitive since integer indexes also do dimension reduction on Nim arrays of arrays and seqs of seqs. And since slices on tensors have to keep the dimension if the slice's range is greater than a single value, they naturally signal dimension preservation. So for me, these two ways of indexing seem like a nice way of differentiating the two different keepdim options, that integer indexes result in dimension reduction and that slices result in dimension preservation.

Clonkk commented 1 year ago

Closing this ticket since we decided to keep PyTorch behaviours.

Like suggested, an upgrade / improvement over the indexing macros is always possible