-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
fix: Fix Ivy Failing Test: paddle - elementwise.multiply #28056
fix: Fix Ivy Failing Test: paddle - elementwise.multiply #28056
Conversation
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.
Hii @sgalpha01 can you please remove the changes you made in importing modules.
from ivy import promote_types_of_inputs | ||
from ivy.func_wrapper import ( | ||
with_unsupported_device_and_dtypes, | ||
with_supported_device_and_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.
with_supported_device_and_dtypes, | |
with_supported_device_and_dtypes, you don't need to change this. |
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.
Hey @samthakur587, the changes are simply reordering the functions imported in ascending order, which was automatically done by black
. No imports are removed or added. Please let me know if you want me to revert the imports to their initial state.
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.
can you please just update your branch to latest commit. i want to check it's still coming or not. just for review. 😄
with_supported_device_and_dtypes, | |
with_supported_device_and_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.
Hello @samthakur587, I updated my branch to the latest commit. I'll now make the required changes you requested in subsequent messages.
"float16", | ||
) | ||
}, | ||
{"2.6.0 and below": ("int8", "uint8", "int16", "float16", "bfloat16")}, | ||
backend_version, |
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.
it's good and working fine. but i think it's looks more understandable if you add @supported_dtype(float32, float64, int32, int64 ,complex).
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.
and can you please implement code for complex dtype at paddle backend.
backend_version, | |
backend_version, |
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.
Adding test support for bool
also as it works in the main library.
>>> x = paddle.to_tensor([[True, False], [False, True]])
>>> y = paddle.to_tensor([[False, False], [True, False]])
>>> x * y
Tensor(shape=[2, 2], dtype=bool, place=Place(cpu), stop_gradient=True,
[[False, False],
[False, False]])
Made the changes to handle complex
dtype. Please review it.
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.
Hii @sgalpha01 the changes you made looking good to me. i have tested it's passing all the tests. 👍🏾
…le_elementwise_multiply
…le_elementwise_multiply
…le_elementwise_multiply
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.
one last change and your PR ready to merge. as i have seen that you added the support of bool dtype and ivy test_case by default it uses tf as ground truth backend and tf doesn't support the bool dtype as a result the bool dtype is not tested at any backend. 😄 so you have to make some changes in test_function.
after making this changes check it is passing all the tests at local.
change helper dtype to valid and ground truth to torch. |
@samthakur587, I made the changes in the test file and ran the tests locally. Please review it. |
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.
LGTM 🚀
PR Description
paddle
doesn't supportbfloat16
, that's why tests were failing.Related Issue
Closes #28055
Checklist
Socials