-
Notifications
You must be signed in to change notification settings - Fork 434
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
Support unique_with_counts #2195
Conversation
b269fef
to
ca567ab
Compare
Signed-off-by: Salvetti, Francesco <[email protected]>
Signed-off-by: Salvetti, Francesco <[email protected]>
Signed-off-by: Salvetti, Francesco <[email protected]>
Signed-off-by: Salvetti, Francesco <[email protected]>
4c4b978
to
1e2e4b1
Compare
Signed-off-by: Salvetti, Francesco <[email protected]>
Signed-off-by: Salvetti, Francesco <[email protected]>
Use LANCZOS instead. Signed-off-by: Jay Zhang <[email protected]>
…/tensorflow-onnx into support-UniqueWithCounts
ctx.insert_new_node_on_output("Cast", new_node.output[0], to=inp_dtype, | ||
name=utils.make_name(node.name) + "_cast") | ||
|
||
# cast idx and counts if needed |
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.
Where do we cast 'counts'?
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.
In the the for loop (Line 2403). If the node has 2 outputs (TF "Unique" op) we cast onnx output 2 ("inverse_indices"). If the node has 3 outputs (TF "UniqueWithCounts" op) we cast onnx output 2 ("inverse_indices") and output 3 ("counts"). Either case, we skip onnx output 1 ("indices") since it is not present in the TF ops.
|
||
# cast idx and counts if needed | ||
out_dtype = node.get_attr_value('out_idx') | ||
if out_dtype != TensorProto.INT64: |
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.
Since out_idx is optional, what will happen here if the output of Unique onnx op contains only 1 element? Line 2402 may still be true, but it is not what we expect, right?
I agree accessing these attributes by a name is more meaningful, but probably it is more safe to access them by output lenght and index. Thoughts?
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.
According to TF API 'out_idx' should be either INT32 or INT64, and has a default value set to INT32, so I would expect that the attribute is always set to one of the two dtypes. And since onnx Unique op always provides "inverse_indices" and "counts" as INT64 they should be casted if 'out_idx' is different from INT64.
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.
LGTM, thanks!
Add mapping for tf.unique_with_counts