Skip to content

Conversation

xadupre
Copy link
Collaborator

@xadupre xadupre commented Sep 29, 2020

No description provided.

Signed-off-by: xavier dupré <[email protected]>

@tf_op("InvertPermutation")
class InvertPermutationOp:
supported_dtypes = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems unused. What is it for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The operator only supports these types. I was that on other operators. I assumed some verification would automatically happen. Based on your remark, I assume not. I'll do that then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just searched the repo for supported_dtypes. It is usually declared in a function and then iterated through for casting purposes. I think you might have seen it and mistakenly thought it should be assigned in a class. You defined it in RFFT, ComplexAbs, and Atan2 but I don't think it should be included. Sorry I didn't catch that sooner.

outputs = [topk_unused, utils.port_name(topk_indices, 1)]
topk_node = ctx.make_node(
"TopK", inputs=[neg_node.output[0], shape_node.output[0]],
name=utils.make_name(node.name + '_topk'), outputs=outputs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The output names should be node_name:0 and node_name:1 which I think make_node will assign automatically. make_name is for making node names, not output names, I think.

Signed-off-by: xavier dupré <[email protected]>
@xadupre xadupre merged commit ba533d0 into onnx:master Oct 6, 2020
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.

3 participants