-
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
fix: Fixed stat.paddle.mean
for all backends
#28087
fix: Fixed stat.paddle.mean
for all backends
#28087
Conversation
Hey, @Sarvesh-Kesharwani thanks for the review. I'm not sure, how many examples you set while testing, I tested with 25, 50, 100 and 150 examples.
Although when I tested with 100 examples, only But, it is actually not any problem with code, it's just the accuracy of the result. You can see that the test is failing due to a minute difference of 10^(-3) difference in the outputs from the 2 backends, Due to this small difference some tests are failing when we set the number of examples too high. I guess, that is the reason, Even in the CI, the test is not shown as failed because in the CI tests are ran with only 25 examples. So, I guess we can merge this, no need to change anything. Please let me know your opinion. |
@@ -26,7 +26,7 @@ def test_paddle_mean( | |||
backend_fw, | |||
test_flags, | |||
): | |||
input_dtype, x, axis = dtype_and_x | |||
input_dtype, x, axis, dtype3, where = dtype_and_x |
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.
These seem unnecessary
input_dtype, x, axis, dtype3, where = dtype_and_x | |
input_dtype, x, axis = dtype_and_x |
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, you can see in this logs, that is the reason I added those 2 variables.
This following function returns 5 values for the case of "mean". So, those 2 variables must be there
https://github.com/unifyai/ivy/blob/995886245e9cde9885ee02b5a4ba4668427230a2/ivy_tests/test_ivy/test_functional/test_core/test_statistical.py#L40
https://github.com/unifyai/ivy/blob/995886245e9cde9885ee02b5a4ba4668427230a2/ivy_tests/test_ivy/test_functional/test_core/test_statistical.py#L98
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.
Oh, I thought it won't unpack the rest like JS (sorry have been off Python for a while). But why don't we use this?
input_dtype, x, axis, dtype3, where = dtype_and_x | |
input_dtype, x, axis = dtype_and_x[:2] |
Not sure if I'm slicing correctly though
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.
Done👍🏻
Also, regarding the current issues with the failing tests. They shouldn't fail even if the code appears right, the precision issue should be handled accordingly @Sai-Suraj-27. And make sure examples cited by @Sarvesh-Kesharwani are passing |
Hey, @KareemMAX I know precision should be handled but since the test was passing in CI also. I thought it's okay. Actually this is a clear example of this 2nd case in the docs. Thanks @Sarvesh-Kesharwani for spotting 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.
LGTM, waiting for @Sarvesh-Kesharwani's approval to merge
Hey, @Sarvesh-Kesharwani. Can you please look into the PR once and merge it if you think it's good 😄. |
Okay let me test once, then I'll merge it. |
Hi @Sai-Suraj-27, |
33369c8
into
Transpile-AI:main
PR Description
Fixed
stat.paddle.mean
for all backendsRelated Issue
Closes #28082
Closes #28083
Closes #28084
Closes #28085
Closes #28086
Checklist
Socials