Clearer description of Slice #3908

diyessi · 2021-12-24T02:24:03Z

Signed-off-by: Scott Cyphers cyphers@lightmatter.co

Description

Reworked the description of Slice.

Motivation and Context

The intended behavior for negative values is unclear; the referenced NumPy documentation is equally unclear, as is Python's documentation.

gramalingam · 2022-01-04T19:25:12Z

onnx/defs/tensor/defs.cc

@@ -769,18 +769,31 @@ ONNX_OPERATOR_SET_SCHEMA(
 static const char* Slice_ver13_doc = R"DOC(
 Produces a slice of the input tensor along multiple axes. Similar to numpy:
 https://docs.scipy.org/doc/numpy/reference/arrays.indexing.html
-Slices uses `starts`, `ends`, `axes` and `steps` inputs to specify the start and end


I wonder if https://numpy.org/doc/stable/user/basics.indexing.html?highlight=slice#slicing-and-striding is a better link?

A link to NumPy would make more sense for something preceded by "Similar to numpy".

gramalingam · 2022-01-04T19:27:31Z

onnx/defs/tensor/defs.cc

+
+All negative values in `starts[i]` and `ends[i]` have `dims[axes[i]]` added to them,
+where `dims` are the dimensions of `input`. Then `start[axes[i]]` is the adjusted
+`starts[i]` clamped into range `[0, dims[axes[i]]]`. The clamping for the adjusted


Should that be dims[axes[i]]-1 ?

Thanks for the clarification. The spec is somewhat ambiguous. One interpretation would be that the sliced indices are all the values "start + jstep" that lie within the valid range obtained by clamping. The other interpretation would be to clamp start first, and generate values "clamped(start) + jstep". Do we know which is intended? Thanks!

It's dim[axes[i]] since you want -1 to go to the last element.

The problem with negative steps is that there is really no clean way to stop at 0 since end is exclusive and negative values get shifted by the dimension size; you need to pick a really big negative end that is still below 0 when shifted by the size.

There is some code just below the doc that does some of this stuff; it looked similar to what I had implemented in our fork of glow based on experiments with numpy.

I see what you meant now. It gets a little confusing with [] being closed intervals, start being inclusive and end being exclusive. I also made the code and the text consistent.

Thanks. Everything seems fine, except one point I am confused about: what happens if starts[i] = dims[axes[i]] and steps is negative ? Your new version says it will get clamped into the value dims[axes[i]] - 1. I tried it is onnxruntime, and this is what happens. But, then, this line in the shape-inference code doesn't seem to be consistent with that:

onnx/onnx/defs/tensor/defs.cc

Line 824 in 50a1981

start = clamp(start, 0, input_rank);

In the PR I had changed that to input_rank - 1.

BTW, when looking for that I noticed a one-word typo/omitto which I just fixed.

Sorry, my fault. I saw that, but it didn't register, I thought the changes were only in the documentation, not the code. LGTM, thanks!

CLAassistant · 2022-01-19T20:15:48Z

All committers have signed the CLA.

jcwchen

Thanks for the clarification! Please use update_doc.sh or update_doc.bat to update Operator.md as well.

gramalingam · 2022-03-16T00:27:24Z

Thanks Jacky for the update!

jcwchen · 2022-03-25T17:13:57Z

@diyessi Do you have bandwidth to finish this PR? If not, please allow me to send a duplicate PR of yours and go forward. Thank you for your contribution.

diyessi · 2022-03-25T17:39:42Z

I might today; I need to re-figure out how to run the ONNX stuff (the README needs some updates). If I don't, feel free to jump in.

Do I just need to merge from main, run update_doc.sh and push back?

jcwchen · 2022-03-25T17:49:34Z

Do I just need to merge from main, run update_doc.sh and push back?

Thank you for the quick reply. Other than those, you will also need to fix the DCO issue because some of your commits were not sign-offed. A faster way to solve it is to squash all your old commits into one (for instance, git reset softly to the base and make/sign-off a single commit with your whole change).

diyessi · 2022-03-25T21:33:16Z

@jwchen I think it's ready now.

jcwchen · 2022-03-25T21:50:41Z

@jwchen I think it's ready now.

Thank you for the quick update! Sorry that I might not be clear: actually using update_doc.sh will update Operators.md and Changelog.md. The CI failed because Changelog.md was not updated. Please use update_doc.sh to update Changelog.md as well and the PR should be ready to go.

Signed-off-by: Scott Cyphers <cyphers@lightmatter.co>

diyessi · 2022-03-25T22:17:04Z

@jwchen I think it's ready now.

Thank you for the quick update! Sorry that I might not be clear: actually using update_doc.sh will update Operators.md and Changelog.md. The CI failed because Changelog.md was not updated. Please use update_doc.sh to update Changelog.md as well and the PR should be ready to go.

It actually updates a lot of things when I run it. Hopefully this time it will all be good.

onnx/defs/tensor/defs.cc

hariharans29 · 2022-04-05T23:37:38Z

docs/Changelog.md

+  The clamping for the adjusted `ends[i]` depends on the sign of `steps[i]` and must
+  accommodate copying 0 through `dims[axes[i]]` elements, so for positive stepping
+  `end[axes[i]]` is clamped to `[0, dims[axes[i]]]`, while for negative stepping it
+  is clamped to `[-1, ends[i]-1]`.


Should this read dims[axes[i]]-1 or am I mis-understanding something ?

Starts are inclusive, ends are exclusive, so the slice covers indices {i | start <= i < end}.
If it still seems wrong or you see a way to make it clearer, let me know.

Sorry for not being clear enough. I think the logic is right. Looking at it, it seems inconsistent with the way it is worded.

"...so for positive stepping ends[axes[i]] is clamped to [0, dims[axes[i]]]..."

Looking at the above line, I would have thought, the last set of words should be .... is clamped to [-1, dims[axes[i]] - 1] instead of the ends you are referencing. Does this make sense or am I on the wrong track ? :)

Let's say we have a tensor X[5]. Then a slice(X, start=0, end=0) is empty, slice(X, start=0, end=5) is all of X, and slice(X, start=0, end=101086) is the same as slice(X, start=0, start=5).
In ranges, [ and ] are inclusive, ( and ) are exclusive.

I agree with everything above.

I was just pointing out the clamping wording while negative stepping:
[-1, ends[i]-1] -> [-1, dims[axes[i]] - 1] ?

And change the textual description accordingly. (Since we are not bumping the opset version, this PR should be only a clarification, not a change in behavior.)

Does that sound reasonable? Thanks!

I'm not seeing how the step > 0 case above is correct; start is inclusive, which is [0, input_rank), not [0, input_rank].

NumPy does behave like the original.

@gramalingam : I think reverting to the old shape inference will be a good idea. It atleast maintains status quo and as you rightly say changing any behavior without an opset bump up is not a good idea.

@diyessi : Try this example on Numpy (this is for step > 0),
x = np.array([1, 2, 3])
x[5:6:1]
array([], dtype=int32)

If start had been clamped to an inclusive value as you are suggesting, with the ends clamping the way we all agree it to be, the result would actually be non-empty

Signed-off-by: Scott Cyphers <cyphers@lightmatter.co>

diyessi requested a review from a team as a code owner December 24, 2021 02:24

gramalingam reviewed Jan 4, 2022

View reviewed changes

gramalingam added the spec clarification Clarification of the ONNX spec needed label Jan 4, 2022

diyessi requested a review from gramalingam January 10, 2022 16:51

gramalingam approved these changes Feb 1, 2022

View reviewed changes

jcwchen reviewed Feb 2, 2022

View reviewed changes

diyessi force-pushed the cyphers/slice branch from 9810e8d to af151d5 Compare March 25, 2022 21:32

Clearer description of Slice

3841a0b

Signed-off-by: Scott Cyphers <cyphers@lightmatter.co>

diyessi force-pushed the cyphers/slice branch from af151d5 to 3841a0b Compare March 25, 2022 22:06

jcwchen reviewed Mar 25, 2022

View reviewed changes

onnx/defs/tensor/defs.cc Show resolved Hide resolved

jcwchen approved these changes Mar 25, 2022

View reviewed changes

jcwchen merged commit 61c36aa into onnx:main Mar 26, 2022

hariharans29 reviewed Apr 5, 2022

View reviewed changes

This was referenced Apr 6, 2022

Seeking clarity on Slice operator's clamping #4115

Closed

Incorrect wording in Slice operator's spec #4116

Closed

Update to Slice microsoft/onnxruntime#11132

Open

wschin pushed a commit to wschin/onnx that referenced this pull request Apr 12, 2022

Clearer description of Slice (onnx#3908)

d1f5b82

Signed-off-by: Scott Cyphers <cyphers@lightmatter.co>

broune pushed a commit to broune/onnx that referenced this pull request May 6, 2023

Clearer description of Slice (onnx#3908)

2eb0a5f

Signed-off-by: Scott Cyphers <cyphers@lightmatter.co>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clearer description of Slice #3908

Clearer description of Slice #3908

diyessi commented Dec 24, 2021

gramalingam Jan 4, 2022

diyessi Jan 6, 2022

gramalingam Jan 4, 2022

gramalingam Jan 4, 2022

diyessi Jan 6, 2022

diyessi Jan 10, 2022

gramalingam Jan 27, 2022

diyessi Feb 1, 2022

gramalingam Feb 1, 2022

CLAassistant commented Jan 19, 2022 •

edited

Loading

jcwchen left a comment

gramalingam commented Mar 16, 2022

jcwchen commented Mar 25, 2022

diyessi commented Mar 25, 2022

jcwchen commented Mar 25, 2022

diyessi commented Mar 25, 2022

jcwchen commented Mar 25, 2022

diyessi commented Mar 25, 2022

hariharans29 Apr 5, 2022

diyessi Apr 6, 2022

hariharans29 Apr 6, 2022 •

edited

Loading

diyessi Apr 6, 2022

hariharans29 Apr 6, 2022

gramalingam Apr 6, 2022

gramalingam Apr 6, 2022

diyessi Apr 6, 2022 •

edited

Loading

diyessi Apr 6, 2022

hariharans29 Apr 6, 2022 •

edited

Loading

Clearer description of Slice #3908

Clearer description of Slice #3908

Conversation

diyessi commented Dec 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jan 19, 2022 • edited Loading

jcwchen left a comment

Choose a reason for hiding this comment

gramalingam commented Mar 16, 2022

jcwchen commented Mar 25, 2022

diyessi commented Mar 25, 2022

jcwchen commented Mar 25, 2022

diyessi commented Mar 25, 2022

jcwchen commented Mar 25, 2022

diyessi commented Mar 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hariharans29 Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diyessi Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hariharans29 Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

CLAassistant commented Jan 19, 2022 •

edited

Loading

hariharans29 Apr 6, 2022 •

edited

Loading

diyessi Apr 6, 2022 •

edited

Loading

hariharans29 Apr 6, 2022 •

edited

Loading