Add AttributeHasValue op #4525
Add AttributeHasValue op #4525
Conversation
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
onnx/defs/tensor/defs.cc
Outdated
| AttributeHasValue, | ||
| 18, | ||
| OpSchema() | ||
| .SetDoc(R"DOC(Returns which elements of the input are NaN.)DOC") |
There was a problem hiding this comment.
Change to "Returns true iff at least one of the attribute-value is specified."
onnx/defs/tensor/defs.cc
Outdated
| const size_t numOutputs = ctx.getNumOutputs(); | ||
| if (numOutputs != 1) { | ||
| fail_type_inference("AttributeHasValue is expected to have 1 output."); | ||
| } |
There was a problem hiding this comment.
The checks above are unnecessary and can be omitted. The default-checker will already check this from the opschema information.
onnx/defs/tensor/defs.cc
Outdated
| auto ints_attr_proto = ctx.getAttribute("ints"); | ||
| bool has_ints = (nullptr != ints_attr_proto) && ints_attr_proto->ints_size() > 0; | ||
| auto int_attr_proto = ctx.getAttribute("int"); | ||
| bool has_int = (nullptr != int_attr_proto) && int_attr_proto->has_i(); |
There was a problem hiding this comment.
I guess you will add a check for all other attributes as well?
There was a problem hiding this comment.
May be something like:
for (const char *name : { "int", "ints", ... })
if ctx.getAttribute(name) != nullptr {
has_attr = true;
break;
}There was a problem hiding this comment.
Yes I will find a way to iterate all types defined in proto. For now, I just want to covert ints so that I can validate it in converting reduce ops to function ops.
onnx/defs/tensor/defs.cc
Outdated
| FunctionProto& functionProto) { | ||
| FunctionBuilder builder(functionProto); | ||
| auto ints_attr_proto = ctx.getAttribute("ints"); | ||
| bool has_ints = (nullptr != ints_attr_proto) && ints_attr_proto->ints_size() > 0; |
There was a problem hiding this comment.
I think we should omit the second check on ints_size?
There was a problem hiding this comment.
I think I added the check due to test failure. Let me double check.
onnx/defs/tensor/defs.cc
Outdated
| // ToTensor does not work with boolean element type. Need to create a Constant int | ||
| // and then Cast to tensor of boolean. | ||
| if (has_ints) { | ||
| builder.Const("output0", 1); |
There was a problem hiding this comment.
I think something like
builder.Add("output0 = Constant < value = bool {1} > ()");may work?
| **kwargs, | ||
| ) | ||
|
|
||
| output = np.array(True) |
There was a problem hiding this comment.
May be replace True by bool(kwargs) ... then you can use the same test-case with no-attributes as well as one-or-more attributes.
onnx/defs/tensor/defs.cc
Outdated
| if (has_ints) { | ||
| builder.Const("output0", 1); | ||
| } else if (has_int) { | ||
| builder.Const("output0", 1); |
There was a problem hiding this comment.
Merge this condition (has_int) with the condition of has_ints?
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
|
This pull request introduces 1 alert when merging 5e56af2 into 895593a - view on LGTM.com new alerts:
|
| @@ -23,7 +26,7 @@ def test_one_attribute(name, **kwargs): | |||
| node, | |||
| inputs=[], | |||
| outputs=[output], | |||
| name="test_attribute_has_value_{name}_false".format(name=name), | |||
| name=f"test_attribute_has_value_{name}_false", | |||
There was a problem hiding this comment.
May be change to test_attribute_has_{name}_false ? Or, omit the value_ part in the call-site. Otherwise, we will get value_value_int.
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu liqfu@microsoft.com
Description
Add AttributeHasValue op
Motivation and Context
This PR is to solve part of the issue (AttributeHasValue and StaticOptionalHasValue): ##3921
This is an op to help convert primary ops with optional attribute to simply function ops.
This op is a function op thus not a burden for runtimes. It is not simple function ops though.