Adding Optional ops to opset 15 #3567
Conversation
Signed-off-by: neginraoof <neginmr@utexas.edu>
neginraoof
force-pushed
the
neraoof/optional
branch
from
b07b38b
to
c0180a6
Compare
|
@postrational can you help review this. We need to get this in onnx 1.10 These ops were reviewed and validated in onnxruntime as contrib ops: microsoft/onnxruntime#7946 |
|
|
||
| def optional_has_element_reference_implementation(optional): # type: (Optional[np.ndarray]) -> np.ndarray | ||
| if optional is None: | ||
| return np.array(False) |
There was a problem hiding this comment.
Does this map to a one-dimensional tensor in ONNX? I think we want a scalar (zero-dimensional tensor), right?
There was a problem hiding this comment.
This is a zero dimension numpy scalar. The ONNX model output value info does not have shape (only boolean dtype).
Signed-off-by: neginraoof <neginmr@utexas.edu>
neginraoof
force-pushed
the
neraoof/optional
branch
from
f47e42f
to
4c14d2e
Compare
Signed-off-by: neginraoof <neginmr@utexas.edu>
| def _extract_value_info(input, name, type_proto=None): # type: (Union[List[Any], np.ndarray, None], Text, TypeProto) -> onnx.ValueInfoProto | ||
| if type_proto is None: | ||
| if isinstance(input, list): | ||
| elem_type = onnx.mapping.NP_TYPE_TO_TENSOR_TYPE[input[0].dtype] |
There was a problem hiding this comment.
please add this comment back :
TODO: Account for recursive sequence case. Right now, this function supports
Sequences of Tensors.
we are still only supporting Sequence of tensors
There was a problem hiding this comment.
In the case where type_proto is None there is no else block to set type_proto... meaning if control does not enter
if isinstance(input, list) or elif isinstance(input, np.ndarray) or np.isscalar(input) then type_proto will be None when the control reaches line 118.
There was a problem hiding this comment.
how will this work in the case when optional proto holds None value? type_proto will be None?
There was a problem hiding this comment.
Agree. Better to have an assertion/check if the value is None.
There was a problem hiding this comment.
@askhade I don't think we need the comment back. The function now does support sequence of sequence or other recursive types if the type_proto for such type is provided. So users can create this kind of type_proto and then use _extract_value_info to create the value info.
There was a problem hiding this comment.
@askhade In the case where optional_proto holds None, we rely on the user to provide a corresponding type_proto. For a None tensor, the type_proto has elem_type (depending on the None tensor data type).
I'll add a check to fail if both input and type_proto are None.
Signed-off-by: neginraoof <neginmr@utexas.edu>
Signed-off-by: neginraoof <neginmr@utexas.edu>
neginraoof
force-pushed
the
neraoof/optional
branch
from
181006b
to
f77ff0c
Compare
Signed-off-by: neginraoof <neginmr@utexas.edu>
neginraoof
force-pushed
the
neraoof/optional
branch
3 times, most recently
from
6c73720
to
75e3475
Compare
neginraoof
force-pushed
the
neraoof/optional
branch
from
75e3475
to
6d25dd7
Compare
neginraoof
force-pushed
the
neraoof/optional
branch
2 times, most recently
from
35549fa
to
d629005
Compare
neginraoof
force-pushed
the
neraoof/optional
branch
from
d629005
to
61c5107
Compare
neginraoof
force-pushed
the
neraoof/optional
branch
from
f7d7eca
to
ea2ee70
Compare
…tional Signed-off-by: neginraoof <neginmr@utexas.edu>
neginraoof
force-pushed
the
neraoof/optional
branch
from
ea2ee70
to
af98e2a
Compare
Signed-off-by: neginraoof <neginmr@utexas.edu>
Description
Adding operators for optional type to opset 15:
Optional (constructor)
OptionalHasElement
OptionalGetElement
Motivation and Context
These ops are necessary for enabling export of customer models with optional type.