-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Implemented and tested Rot90 for Paddle frontends #17708
Conversation
Close #16746 |
If you are working on an open task, please edit the PR description to link to the issue you've created. For more information, please check ToDo List Issues Guide. Thank you 🤗 |
Hi! @DragosStoican Could you please help me check about my code? |
init_tree="paddle.to_tensor", | ||
method_name="rot90", | ||
dtype_m_k_axes=_get_dtype_values_k_axes_for_rot90( | ||
available_dtypes=helpers.get_dtypes("numeric"), |
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.
We should be ideally testing for all valid dtypes, and not only a subset.
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.
The numeric has included float, integer, bool types that are all supported by Rot90. you can check https://docs.python.org/3/library/stdtypes.html#numeric-types-int-float-complex. Please free free to ask me any questions. Thank you!
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.
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.
Hi @XinyuanWang283 we would prefer to use the argument "valid" in the helpers.get_dtypes
function. When using the "valid" argument the tests are created according to the supported dtypes you've defined with the decorator
@with_supported_dtypes
.
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.
Thank you very much for your guidance. @DragosStoican Sorry, I made a mistake. I have fixed it. Could you help me review it again. Thanks!
Thank you very much for your contribution and cooperation @XinyuanWang283. I'm happy to get this merged now! |
Close#16746