Skip to content

Conversation

@vinayakdsci
Copy link
Contributor

Adds OnnxToTorch lowering for Onnx.STFT op.

@vinayakdsci
Copy link
Contributor Author

cc @vivekkhandelwal1.

Copy link
Collaborator

@vivekkhandelwal1 vivekkhandelwal1 left a comment

Choose a reason for hiding this comment

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

Overall, there are too many control flow paths many of which can be combined with slight modifications, please do it wherever possible to make the flow simpler.

@vinayakdsci
Copy link
Contributor Author

Hi @vivekkhandelwal1, sorry for so many control flow paths. I have fixed all the readabiity issues, and in the AtenSTFTOp, now there is only one condition checking if onesided is true or false, which I could not avoid.

I apologize for the rebase, it was necessary to resolve the merge conflicts. Thanks for the review!

Copy link
Collaborator

@vivekkhandelwal1 vivekkhandelwal1 left a comment

Choose a reason for hiding this comment

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

Please address the minor comments, then the PR is ready to go.

@vinayakdsci
Copy link
Contributor Author

@vivekkhandelwal1 all comments have been addressed in the latest commit. Thanks for the review!

@vivekkhandelwal1 vivekkhandelwal1 merged commit 0234040 into llvm:main Jun 25, 2024
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.

2 participants