Relax IF's shape inference rule#2345
Conversation
|
|
||
| # Create a simple If node where the 'then' subgraph adds to the current value, and the 'else' subgraph | ||
| # subtracts. | ||
| # can't use self._make_graph for the subgraphs as that add more inputs for the Reshape operations it inserts. |
There was a problem hiding this comment.
Wording is awkward here, perhaps avoid calling self._make_graph here for subgraph generation as extra reshape nodes will be created
There was a problem hiding this comment.
I feel the current description is more precise. From this description, adding Reshape itself is not a problem, but adding input required by Reshape causes the problem.
| auto* if_output = ctx.getOutputType(i); | ||
| *if_output = *then_output; | ||
|
|
||
| if (then_output->has_tensor_type()) { |
There was a problem hiding this comment.
Suggest to avoide has_type() function. Simply type() would return a default instance which would have value_case() NOT_SET
There was a problem hiding this comment.
I feel it's a bigger problem in the whole codebase. If you agree, I'd like to open another issue for addressing it globally.
| inline bool checkDimensionCompatibility( | ||
| const TensorShapeProto_Dimension& source_dim, | ||
| const TensorShapeProto_Dimension& target_dim) { | ||
| if (source_dim.has_dim_value() && target_dim.has_dim_value()) { |
There was a problem hiding this comment.
Use switch statement instead of has_*() functions which are absent in proto3. You want this to run in ML.NET.
There was a problem hiding this comment.
I believe it is not a problem with "one of" fields … the has_* functions are generated even in proto3 for "one of", if I am not mistaken
There was a problem hiding this comment.
See https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#oneof-numeric-fields and https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#oneof-embedded-message-fields … both proto2 and proto3 support "has_*" for one of fields. It is only for primitive-type fields that are not "one of" we have the proto2/proto3 issue (IIUC).
* Relax If's shape inference rule * Make shape inference tests ok and move code to the right place * Add document changes * Update onnx/defs/controlflow/defs.cc * Update onnx/defs/controlflow/defs.cc * Address comments * Address comments * Address comments * Fix shape inference test * Disable a type check * Address a comment * Update defs.cc * Update Changelog.md * Update Operators.md
* Relax IF's shape inference rule (#2345) * Relax If's shape inference rule * Make shape inference tests ok and move code to the right place * Add document changes * Update onnx/defs/controlflow/defs.cc * Update onnx/defs/controlflow/defs.cc * Address comments * Address comments * Address comments * Fix shape inference test * Disable a type check * Address a comment * Update defs.cc * Update Changelog.md * Update Operators.md * Bump NMS version for avoiding regression in existing models (#2348) * Bump NMS version for avoiding regression in existing models * Bring old logic back * Fix spec and shape inference for Unsqueeze op (#2347) * Fix spec for Unsqueeze * Update Changelog.md * Refine doc * Refine * PR comments * Update Changelog.md * Update shape inference test file
* Relax If's shape inference rule * Make shape inference tests ok and move code to the right place * Add document changes * Update onnx/defs/controlflow/defs.cc * Update onnx/defs/controlflow/defs.cc * Address comments * Address comments * Address comments * Fix shape inference test * Disable a type check * Address a comment * Update defs.cc * Update Changelog.md * Update Operators.md
This PR allows If to produce different shapes in
thenandelsebranches. It makes the uses ofIfeasier. It will also fix a shape inference problem in SSD objection detection model.