Skip to content
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

Merged
merged 11 commits into from
Jul 26, 2023
Merged

Conversation

f-salvetti
Copy link
Contributor

Add mapping for tf.unique_with_counts

@f-salvetti f-salvetti force-pushed the support-UniqueWithCounts branch from b269fef to ca567ab Compare June 30, 2023 13:35
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]>
@f-salvetti f-salvetti force-pushed the support-UniqueWithCounts branch from 4c4b978 to 1e2e4b1 Compare June 30, 2023 14:23
Signed-off-by: Salvetti, Francesco <[email protected]>
@f-salvetti f-salvetti marked this pull request as ready for review June 30, 2023 15:20
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
Copy link
Collaborator

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'?

Copy link
Contributor Author

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:
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@fatcat-z fatcat-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@fatcat-z fatcat-z merged commit 6cdb7e3 into onnx:main Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants