-
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
Append dim even both dim value and param are not set #3828
Conversation
Signed-off-by: Joe <joe@preferred.jp>
b4fdd94
to
6623505
Compare
Curious - Are you using ONNX's graph level shape inference? meaning are you using onnx shape inference apis? When using onnx graph level shape inference I don't know how you could land in this situation this because - either the input data is an initializer in which case all dimensions are dim values or the input data is dynamic in which case graph level shape inference will create a symbol for every unknown dim... (https://github.com/onnx/onnx/blob/master/onnx/shape_inference/implementation.cc#L141) However, not all runtimes implement symbol generation and therefore it may be best to include your fix. |
onnx/defs/data_propagators.h
Outdated
} else if (dim.has_dim_param()) { | ||
tsp.mutable_dim()->Add()->set_dim_param(dim.dim_param()); | ||
} | ||
tsp.mutable_dim()->Add()->CopyFrom(dim); |
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.
can you simply add a dim? tsp.mutable_dim()->Add() instead of Copy
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.
Added a comment
@xuzijian629 : The PR looks good to me. I added a minor comment. As soon as you address it we can merge this. Thanks for your contribution. |
@xuzijian629: Any updates? |
Any updates? |
Sorry, I missed the notification. I will soon make an update 🙇 |
This reverts commit 6623505. Signed-off-by: Joe <joe@preferred.jp>
Yes, I encountered this problem when I was using ONNX's graph level shape inference. I got the bug in the following part of a model. Since I cropped the graph, the inputs of |
I summarized the above explanation into code. Note that before running this, you may need to merge #3784 because the following code uses data propagation for ConstantOfShape. import onnx
from onnx import parser, helper, shape_inference
input = '''
agraph (float[3, n] x) => ()
{
shape = Shape(x)
one = Constant<value = int64 {1}>()
scalar = Gather(shape, one)
subbed = Sub(scalar, one)
unsqueeze_dim = Constant<value = int64[1] {0}>()
unsqueezed = Unsqueeze(subbed, unsqueeze_dim)
concat_out = Concat<axis = 0>(shape, unsqueezed)
out = ConstantOfShape(concat_out)
}
'''
graph = parser.parse_graph(input)
original_model = helper.make_model(graph, producer_name='onnx-examples')
onnx.checker.check_model(original_model)
# Apply shape inference on the model
inferred_model = shape_inference.infer_shapes(original_model, data_prop=True)
onnx.checker.check_model(inferred_model)
print(inferred_model.graph.value_info) With this PR, the output shape of |
Please take a look! @askhade 🙇 |
onnx/defs/data_propagators.h
Outdated
@@ -11,6 +11,8 @@ inline void appendDimToTensorShapeProto(TensorShapeProto& tsp, const TensorShape | |||
tsp.mutable_dim()->Add()->set_dim_value(dim.dim_value()); | |||
} else if (dim.has_dim_param()) { | |||
tsp.mutable_dim()->Add()->set_dim_param(dim.dim_param()); | |||
} else { | |||
tsp.mutable_dim()->Add(); |
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.
Thanks for fixing this! This looks correct. But I think the whole function can be simplified to
* tsp.add_dim() = dim;
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.
Thank you!
Signed-off-by: Joe <joe@preferred.jp>
#3902) * Fix typos (#3894) Signed-off-by: Lewis Tunstall <lewis.c.tunstall@gmail.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Honor existing dim_param in shape inference (#3896) * Honor existing dim_param Before this change, existing dim_param was overwritten by generated symbols like `unk__`. `NonZero` is used to test the behavior. Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com> * Fix test expectation of symbolic_shape_test.py Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com> * Keep the original check as suggested in review Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com> * Simplify the logic of mergeShapesAndTypes Signed-off-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com> Co-authored-by: Ashwini Khade <askhade@microsoft.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * for issue 3849 to confirm that type check is performed during checker.check_model call Signed-off-by: Liqun Fu <liqfu@microsoft.com> * fix path to InferenceError Signed-off-by: Liqun Fu <liqfu@microsoft.com> * replace whitelist by safelist (#3900) Signed-off-by: Alexandre Eichenberger <alexe@us.ibm.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * add checker test case for input type mismatch conflict Signed-off-by: Liqun Fu <liqfu@microsoft.com> * remove old numpy in ipynb (#3916) Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Shapeinf for functions (#3722) * add shape inference for model local functions Signed-off-by: Ashwini Khade <askhade@microsoft.com> * Plus tests and more changes Signed-off-by: Ashwini Khade <askhade@microsoft.com> * fix typo Signed-off-by: Ashwini Khade <askhade@microsoft.com> * Plus updates Signed-off-by: Ashwini Khade <askhade@microsoft.com> * Plus updates per review Signed-off-by: Ashwini Khade <askhade@microsoft.com> * plus updates Signed-off-by: Ashwini Khade <askhade@microsoft.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Fix old ConvTranspose shape inference and softmax upgrader (#3893) * Fixed ConvTranspose-1 shape inference Brings change from #3188 (for ConvTranspose-11) into old shape inference Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com> * Fix softmax adapter Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com> Co-authored-by: G. Ramalingam <grama@microsoft.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Fix Linux i686 Release CI failure due to the latest NumPy (#3918) * 3.8 and 3.9 use numpy 1.21.5 Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * 1.21.4 Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * 6be8011a073feeca08d256ac64335a19fc8eee4c6312668b6781ace71db0de20 Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * 2021-12-19-a4d7f5a Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * 1.21.5 Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * 1.16.6 Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * 1.16.6 Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * do not check generated Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * remove unrelated numpy Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * if Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * remove generation Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * name Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * add back Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * for testing Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * no 1.16.6 Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Remind release manager to remove old onnx-weekly packages after release (#3923) * remind release manager to remove old onnx-weekly packages Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * add steps Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Simplify function definition of context-dependent functions (#3882) * Simplify function definition of context-dependent functions Signed-off-by: Ganesan Ramalingam <grama@microsoft.com> * Add missing parenthesis Signed-off-by: Ganesan Ramalingam <grama@microsoft.com> * Fix errors in function defs Signed-off-by: Ganesan Ramalingam <grama@microsoft.com> * Eliminate unused variable Signed-off-by: Ganesan Ramalingam <grama@microsoft.com> * Add int64 type specifier to literal Signed-off-by: Ganesan Ramalingam <grama@microsoft.com> Co-authored-by: Ashwini Khade <askhade@microsoft.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Migration to using main branch (#3925) * add warning announce Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> * Rename to main branch globally Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Fix the bug of shape in docs (#3927) * fix the bug of shape Signed-off-by: namasikanam <namasikanam@gmail.com> * fix the bug of shape Signed-off-by: namasikanam <namasikanam@gmail.com> Co-authored-by: Ashwini Khade <askhade@microsoft.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Append dim even both dim value and param are not set (#3828) * Append dim even both dim value and param are not set Signed-off-by: Joe <joe@preferred.jp> * Copy dim Signed-off-by: Joe <joe@preferred.jp> * Revert "Copy dim" This reverts commit 6623505. Signed-off-by: Joe <joe@preferred.jp> * Simplify code Signed-off-by: Joe <joe@preferred.jp> Co-authored-by: Ashwini Khade <askhade@microsoft.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * bump to 10.15 in azp (#3941) Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Add explanation to run gtest (#3933) Signed-off-by: Joe <joe@preferred.jp> Co-authored-by: Ashwini Khade <askhade@microsoft.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> * Update TreeEnsembleClassifier and TreeEnsembleRegressor to support tensor as attributes (#3897) * update TreeEnsembleClassifier and TreeEnsembleRegressor Signed-off-by: xavier dupré <xavier.dupre@gmail.com> * change the type of another attribute Signed-off-by: xavier dupré <xavier.dupre@gmail.com> * add missing file Signed-off-by: xavier dupré <xavier.dupre@gmail.com> * update documentation Signed-off-by: xavier dupré <xavier.dupre@gmail.com> * eol Signed-off-by: xavier dupré <xavier.dupre@gmail.com> * add field with _as_tensor Signed-off-by: xavier dupré <xavier.dupre@gmail.com> * fix error messages Signed-off-by: xavier dupré <xavier.dupre@gmail.com> * fix error message Signed-off-by: xavier dupré <xavier.dupre@gmail.com> * fix missing change Signed-off-by: xavier dupré <xavier.dupre@gmail.com> Co-authored-by: xavier dupré <xavier.dupre@gmail.com> Co-authored-by: G. Ramalingam <grama@microsoft.com> Signed-off-by: Liqun Fu <liqfu@microsoft.com> Co-authored-by: lewtun <lewis.c.tunstall@gmail.com> Co-authored-by: Shinichiro Hamaji <shinichiro.hamaji@gmail.com> Co-authored-by: Ashwini Khade <askhade@microsoft.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com> Co-authored-by: Chun-Wei Chen <jacky82226@gmail.com> Co-authored-by: Matteo Salvarezza <matteo.salvarezza@gmail.com> Co-authored-by: G. Ramalingam <grama@microsoft.com> Co-authored-by: Xingyu Xie <namasikanam@gmail.com> Co-authored-by: Joe <joe@preferred.jp> Co-authored-by: Xavier Dupré <xadupre@users.noreply.github.com> Co-authored-by: xavier dupré <xavier.dupre@gmail.com>
Description
appendDimToTensorShapeProto
so that a dimension is appended even when both dim value and param are not known.Motivation and Context
input_data
is not nullptr but its dims have neither dim_values nor dim_params.