-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 shape inference of Split-2 #2549
Conversation
@linkerzhang This is a follow-up PR for #2328 and the same fix for shape inference of Split-11 was done for Split-2. |
onnx/defs/tensor/utils.cc
Outdated
" Value=", | ||
axis); | ||
} | ||
if (allow_negative_axis && axis < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need checking allow_negative_axis here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, sorry for the large latency!
be4e246
to
32697c5
Compare
Hi @shinh, |
This should be same as onnx#2328 Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com>
Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com>
Unlike previous approach which factored out a function to share code among Split-11 and Split-2, this time, I just copied the shape inference of Split-11 to Split-2. This way seems to be more consistent with other operator definitions. At first, I tried to remove the support for negative axis from the one of Split-2 since Split-2 did not mention negative axis. However, it made some tests fail so I decided to handle negative axis even in Split-2. This is quite an old PR. I don't have the model fixed by this PR anymore. If this patch does not look good and you still want to fix Split-2, I'd suggest you create another PR. I'll close this PR then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have confirmed it can fix shape inference bug with Split-2. For instance: https://github.com/onnx/models/blob/master/vision/classification/shufflenet/model/shufflenet-v2-10.onnx
@linkerzhang Could you help to merge this? Thank you. |
} | ||
|
||
const auto& split_dim = shape.dim(axis); | ||
if (!split_dim.has_dim_value()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor point: if the "split" attribute is specified, then we can infer the output dimensions, even if the input-shape does not have a dim-value for the split-axis, isn't that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Do you want to do this in this PR? I'm afraid it'd take time for me to update this PR if we do this in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to do that as a separate PR.
@shinh : thanks for updating the fix. Regarding
that is troubling. I guess there is some mismatch between the spec (which doesn't mention negative |
@gramalingam Makes sense. https://github.com/onnx/onnx/tree/master/onnx/backend/test/data/pytorch-converted/test_GLU This model consists the split node with negative which might break the test previously. |
LGTM. I am afraid it needs your signoff. We can merge it once we have the signoff. |
Co-authored-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com>
Done! |
#2328 did not fix the shape inference of the old Split.
for #1735