to extend OptionalHasElement and OptionalGetElement to accept tensor and sequence types #4421
Conversation
… sequence types Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
| input_type_proto = onnx.helper.make_optional_type_proto(tensor_type_proto) | ||
| node = onnx.helper.make_node( | ||
| 'OptionalHasElement', | ||
| inputs=['optional_input'], |
There was a problem hiding this comment.
Another test-case where inputs=[] or input=[''] would be useful.
How about test-cases for optional_get_element?
onnx/defs/optional/defs.cc
Outdated
| OpSchema() | ||
| .SetDoc(OptionalHasElement_ver1_doc) | ||
| .SetDoc(OptionalHasElement_ver18_doc) | ||
| .Input(0, "input", "The optional input.", "O") |
There was a problem hiding this comment.
The input should be made into an optional input.
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
onnx/backend/test/cmd_tools.py
Outdated
| @@ -48,6 +48,8 @@ def prepare_dir(path: str) -> None: | |||
| output_dir, f'test_data_set_{i}') | |||
| prepare_dir(data_set_dir) | |||
| for j, input in enumerate(inputs): | |||
| if input is None: | |||
There was a problem hiding this comment.
Trying to understand this: this file is about model inputs/outputs, right, and not about node/op inputs/outputs. For model inputs/outputs there is no possibility of optional-input/output of kind 1 (static), right? I don't understand why this is being triggered. (May be some fix in the model-generator is required if it is adding an unnecessary model-input?)
There was a problem hiding this comment.
I am worried things will break if a model has an input X, and there is no corresponding input-file in the test-folder.
| # OptionalHasElement takes a tensor or optional as input | ||
| for input_type_proto in [tensor_type_proto, optional_type_proto]: | ||
| input_name_options = { | ||
| 'empty': 'optional_input', |
There was a problem hiding this comment.
I think the combination of input_type_proto == tensor_type_proto and above ('empty': 'optional_input') is not a valid combination to test? That might also be triggering the change to cmd_tools.py below?
…input Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
onnx/numpy_helper.py
Outdated
| if sys.byteorder == "big": | ||
| # Convert endian from big to little | ||
| convert_endian(tensor) | ||
| if arr is not None: |
There was a problem hiding this comment.
Is this change still required (after eliminating that test-case)? Wondering why this change is needed.
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
…and sequence types (onnx#4421)
Description
to extend OptionalHasElement and OptionalGetElement to accept tensor and sequence types
Motivation and Context
Before the 2 optional ops only accept optional types. This limits its use in cases where input can be both optional and other types. This PR generalizes the 2 ops so it become more flexible.