Skip to content

Conversation

@SamKG
Copy link

@SamKG SamKG commented Aug 3, 2021

This adds support for LessOrEqual (<=) and GreaterOrEqual (>=) ops for ONNX frontend, as introduced in opset 12.

Copy link
Contributor

@mbrookhart mbrookhart 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!

@SamKG
Copy link
Author

SamKG commented Aug 3, 2021

LGTM. Thanks!

thanks!
I wanted to also give adding random ops a try (#8632), but this isn't quite as simple.
Any advice on how to approach it would be appreciated.

@SamKG SamKG force-pushed the main branch 2 times, most recently from db2af02 to e09525b Compare August 3, 2021 17:36
@SamKG
Copy link
Author

SamKG commented Aug 3, 2021

Could a maintainer please re-run the workflow? I fixed a small typo in the commit

Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

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

LGTM, btw if you need to jostle CI you can just git commit -m 'jostle ci' --allow-empty and push.

"Exp": Renamer("exp"),
"Greater": Greater.get_converter(opset),
"Less": Less.get_converter(opset),
"LessOrEqual": LessOrEqual.get_converter(opset),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just use Renamer, you are just forwarding the inputs in the converter?

Copy link
Contributor

Choose a reason for hiding this comment

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

E.g. see #8967

verify_binary_ops("Sum", x, z)
verify_binary_ops("Greater", x, y, "bool")
verify_binary_ops("Greater", x, z, "bool")
verify_binary_ops("GreaterOrEqual", x, y, "bool")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you uncomment the appropriate tests in unsupported_onnx_tests in this file?

@AndrewZhaoLuo
Copy link
Contributor

@SamKG do you plan to try to get this merged?

@SamKG
Copy link
Author

SamKG commented Sep 8, 2021

@SamKG do you plan to try to get this merged?

Hi Andrew,
Yes, I will try to get it merged this weekend - been a bit busy.

@areusch areusch removed their request for review September 14, 2021 15:04
@AndrewZhaoLuo
Copy link
Contributor

@SamKG do you mind if I open up an identical PR and try to merge that? Might need this in soon

@SamKG
Copy link
Author

SamKG commented Sep 20, 2021 via email

@SamKG
Copy link
Author

SamKG commented Sep 22, 2021

Closed as #9066 handles this ( thanks @AndrewZhaoLuo )

@SamKG SamKG closed this Sep 22, 2021
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