Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the starts and sizes definitions of slice op consistent and simplified #369

Closed
huningxin opened this issue Mar 28, 2023 · 4 comments · Fixed by #376
Closed

Make the starts and sizes definitions of slice op consistent and simplified #369

huningxin opened this issue Mar 28, 2023 · 4 comments · Fixed by #376

Comments

@huningxin
Copy link
Contributor

Thanks @fdwr for pointing out the inconsistency of starts and sizes definitions of slice op when revisiting the PR #359 (comment), where Dwayne mentioned:

I just noticed slices parameter starts still has the negative number policy and accepts axis coordinates as long rather than unsigned long; and its sizes parameter has an unusual policy that -1 is special, whereas sizes in resample2D is sequence. Do we want to rationalize these for consistency too, having higher level policy like this determined by the calling framework?

In a followed comment, Dwayne also pointed out that

we see that every other sizes parameter is unsigned long (see below), leaving only slice as sequence sizes.

From my point of view, using unsigned integer for starts and sizes makes sense. It not only would help ensure the consistency across the spec but also would simplify the implementation. The native ML APIs, such as DirectML's DML_SLICE1_OPERATOR_DESC and XNNPACK's xnn_define_static_slice, usually use unsigned integer starts/offsets and sizes.

Another simplification idea is whether the MLSliceOptions should be removed. MLSliceOptions.axes is used to indicate the dimensions of the input shape to which starts and sizes apply. However, native ML APIs, e.g. DirectML's DML_SLICE1_OPERATOR_DESC, XNNPACK's xnn_define_static_slice and MLCompute's MLCSliceLayer, usually expect the starts and offsets for all dimensions to be provided.

With that, a possible simplified slice op definition would be

partial interface MLGraphBuilder {
  // The length of `starts` and `sizes` sequences should equal to the rank of input tensor.
  MLOperand slice(MLOperand input, sequence<unsigned long> starts, sequence<unsigned long> sizes);
};

Thoughts?

@fdwr
Copy link
Collaborator

fdwr commented Mar 28, 2023

Another simplification ... remove ... axes ... and ... all dimensions to be provided.

Thanks for creating the issue and sharing the xnn_define_static_slice and MLCSliceLayer information. I feel comfortable removing the axes parameter now.

whether the MLSliceOptions should be removed

It might be useful to have an options object, if we want to extend it later with window strides/steps (seeing that both MLCompute and DirectML support steps, but evidently not XNNPack). e.g.

dictionary MLSliceOptions {
    sequence<unsigned long> starts;
    sequence<unsigned long> sizes;
    sequence<unsigned long>? steps;
};

MLOperand slice(
    MLOperand input,
    MLSliceOptions options,
);

Though, just appending another sequence<unsigned long> steps parameter later works too:

MLOperand slice(
    MLOperand input,
    sequence<unsigned long> starts,
    sequence<unsigned long> sizes,
    sequence<unsigned long>? steps,
);

Alternately we could do the hybrid option like pad and pass the first two parameters and an options parameter later. So there are at least 3 options here, but I'm not clear what factors in WebNN determine whether parameters should be stuffed inside an options object vs passed directly? Is it mainly driven by optionality of the parameter? I do value consistency between functions, and this 3rd option is consistent with pad.

dictionary MLSliceOptions {
    sequence<unsigned long> steps;
};

MLOperand slice(
    MLOperand input,
    sequence<unsigned long> starts,
    sequence<unsigned long> sizes,
    MLSliceOptions options = {}, // could be added later if needed
);

huningxin added a commit to huningxin/webnn that referenced this issue Apr 10, 2023
1. Change `starts` and `sizes` parameters to be sequence of unsigned long.
2. Remove `MLSliceOptions.axes`.

Fix webmachinelearning#369
@huningxin
Copy link
Contributor Author

huningxin commented Apr 10, 2023

Thanks @fdwr for sharing the insightful thoughts. Very helpful!

Based on the option 3, I made a PR #376 that simplifies the current slice op without adding new MLSliceOptions.steps support as the first step. Please take a look.

Regarding to steps, I found both TensorFlow's strided_slice and DirectML's DML_SLICE1_OPERATOR_DESC support negative stride which causes a reverse slice. Should MLSliceOptions.steps be defined as a sequence of long?

@fdwr
Copy link
Collaborator

fdwr commented Apr 11, 2023

Yep, steps would be signed to mirror the read direction.

I made a PR #367

367? Found it: #376

@huningxin
Copy link
Contributor Author

376? Found it: #376

Yes, it is #376. Sorry, that was my typo.

zolkis pushed a commit to zolkis/webnn that referenced this issue May 15, 2023
1. Change `starts` and `sizes` parameters to be sequence of unsigned long.
2. Remove `MLSliceOptions.axes`.

Fix webmachinelearning#369
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants