changed narrow to standardize negative length and negative offset beh…

[huihuifan]

…avior,

modified documentation to reflect new behavior

[sniklaus]

I did not find any discussion as to why this change has been made, and it breaks backwards compatibility. So may I ask what the rationale behind this change is?

[huihuifan]

Hi @sniklaus:

Two separate things I changed:

1. Fixing the behavior of negative length

2. Modifying the behavior of negative offset. I changed this so that it's possible to select the last element with narrow(1, -1), like this:

   input = torch.Tensor({1, 2, 3, 4, 5})
   nn.Narrow(1, -1, 1):forward(input) will produce 5

I think selecting the last element with -1 makes intuitive sense to me.

These two changes I made at the same time, but it is change 2 that produces the off by 1 error in backward compatibility.

[pavanky]

I think it is a bad idea to be silently breaking APIs without notice or a deprecation warning. It is not documented, and will cause silent failures.

[sniklaus]

This is nevertheless breaking backwards compatibility. Existing code expects a different behavior when supplying a negative length.

nn.PreviousNarrow(1, 2, -1):forward(x):size() != nn.UpdatedNarrow(1, 2, -1):forward(x):size()

I would not be surprised to see an issue report about this soon. I remember we had a discussion about the semantics of the negative length before, which I unfortunately cannot find anymore.

It should however further be discussed as the semantics of the third parameter seem controversial. #917

[sniklaus]

The previous semantics were by the way neat when it comes to cropping an image.

Want to crop 1 pixel left and right / top and bottom? nn.Narrow(1, 2, -2)
Want to crop 2 pixels left and right / top and bottom? nn.Narrow(1, 3, -3)
Want to crop n pixels left and right / top and bottom? nn.Narrow(1, 1+n, -(1+n))

[soumith]

First of all, i'm really sorry about merging this without a depreceated tag etc.
Let me give a bit more context:

negative offsets were introduced on December 12th, 2016: https://github.com/torch/nn/commits/master/Narrow.lua , the commit was from @huihuifan that i ported back into OSS.

As soon as they were introduced, we wanted to make the changes in the current PR, and if we merged the changes within ~2 weeks ish, i thought the whole thing would've been acceptable.

But pushing the diff through had delays, and more than a month later i merged this in. Looking back, not the best of judgements.

We will figure out a way to keep backward compatibility (or) mark deprecation properly before pushing it through.

[sniklaus]

Thank you for quick response @soumith!

My response was mainly due to the change in semantics of the length parameter. The support for negative lengths was added on May 15 of last year.

[jgehring]

The previous semantics were by the way neat when it comes to cropping an image.

I'd consider the previous behavior (from May 2016) broken since you can't mix negative indices and negative length, and a negative length was not really a length any more but rather an index starting at the end of the dimension.

I think one way to solve this would be to use length_v2 or sth for the fixed implementation and fall back to the previous one if the module has length set. This way, models serialized with the previous version will still work correctly. Existing code will have to be changed, though.

[soumith]

@jgehring it's too much of an expectation that all users downstream will change their code referencing nn.Narrow. We cant break userland.

I dont see a good solution to this but here are a couple of not-so-good solutions:

• new name for the new behavior, call it nn.SomethingElse
• constructor argument that controls v2 behavior that'd disabled as a default

[pavanky]

There should also be a consideration for proper deprecation system and including breaking changes in a unified way rather than handling it individually for each module.

I am not sure how feasible this is, but would it be possible to do something like this ?

torch.useBreaking = { 'nn.Narrow', 'nn.SomethingElse' }
require ('nn')

If there were other modules considering breaking changes, they can just look at the list useBreaking (or an equivalent name) and change the behavior if the name is specified in the list.

In the future, if a module is being deprecated for a new behavior, you can start displaying notices saying this will change on a xyz date. Once the date is passed, the new behavior is the default and the old behavior can be enabled through a new list, say torch.useLegacy.

This is a rather crude alternative to the lack of version numbers being used. If there's a proper way to include version numbers along side require, that'd be a better alternative.

[pavanky]

If solving just nn.Narrow is a priority for now, my vote would be nn.Narrow(dimension, offset, length) for old behavior, as opposed to nn.Narrow({dimension, offset, length}) for the new behavior.

This way you can get both behaviors without adding additional parameters.