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

Fix Slice shape inference in the case of dynamic inputs #5475

Merged
merged 6 commits into from Aug 21, 2023
Merged

Fix Slice shape inference in the case of dynamic inputs #5475

merged 6 commits into from Aug 21, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 7, 2023

Description

Slice shape-inference didn't work when the start or stop inputs were not defined.

Motivation and Context

This contributes to making the type and shape inference more complete.

@ghost ghost requested review from a team as code owners August 7, 2023 13:27
Copy link
Member

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be worthy to fix the same issue in Slice-11 and Slice-10 as well?

onnx/onnx/defs/tensor/old.cc

Lines 1050 to 1053 in 025a911

if (!startsInitializer || !endsInitializer || (hasInputShape(ctx, 3) && !ctx.getInputData(3)) ||
(hasInputShape(ctx, 4) && !ctx.getInputData(4))) {
return;
}

@justinchuby
Copy link
Contributor

@onnx/sig-operators-approvers for approval

@justinchuby
Copy link
Contributor

Could you fix dco by signing the new commit?

neNasko1 and others added 2 commits August 10, 2023 19:03
@adityagoel4512
Copy link
Contributor

@justinchuby could we please add this to the merge queue?

@justinchuby
Copy link
Contributor

We need an additional approval from @onnx/sig-operators-approvers

@@ -958,6 +958,11 @@ ONNX_OPERATOR_SET_SCHEMA(

if (!startsInitializer || !endsInitializer || (hasInputShape(ctx, 3) && !ctx.getInputData(3)) ||
(hasInputShape(ctx, 4) && !ctx.getInputData(4))) {
const auto input_rank = ctx.getInputType(0)->tensor_type().shape().dim_size();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks! Unrelated to this change, I wonder if we should change the hasInputShape in the previous two lines to hasInput? We need to only check if the starts/ends inputs are supplied, not whether we know their shape.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed. Reverting

Copy link
Contributor

@gramalingam gramalingam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think we might be able to fix another minor issue at the same time, as mentioned in the comments.

@gramalingam gramalingam added the shape inference Issues related to shape inference label Aug 17, 2023
AtanasDimitrovQC and others added 4 commits August 18, 2023 13:13
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby justinchuby added this pull request to the merge queue Aug 21, 2023
Merged via the queue into onnx:main with commit 9183bbb Aug 21, 2023
35 checks passed
corwinjoy pushed a commit to corwinjoy/onnx that referenced this pull request Sep 5, 2023
### Description
Slice shape-inference didn't work when the start or stop inputs were not
defined.

### Motivation and Context
This contributes to making the type and shape inference more complete.

---------

Signed-off-by: Atanas Dimitrov <atanas.dimitrov@quantco.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Corwin Joy <corwinjoy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shape inference Issues related to shape inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants