Added nanprod to ivy experimental API #21198

BebehCodes · 2023-08-02T15:41:40Z

… unit test

# Conflicts: # ivy/functional/backends/numpy/experimental/statistical.py # ivy/functional/backends/paddle/experimental/statistical.py # ivy/functional/backends/tensorflow/experimental/statistical.py # ivy/functional/backends/torch/experimental/statistical.py

nassimberrada

Hi @BebehCodes, thank you for your contribution ! I've left a few comments could you try to address them ? Overall I think this is going in the right direction we just need to refine some corners. Let me know how this goes. Thanks !

nassimberrada · 2023-08-06T20:47:29Z

ivy/data_classes/container/experimental/statistical.py

+        --------
+        >>> a = ivy.Container(x=ivy.array([[1, 2], [3, ivy.nan]]),\
+                                y=ivy.array([[ivy.nan, 1, 2], [1, 2, 3]])
+        >>> ivy.Container.static_moveaxis(a)


I assume you mean static_nanprod, careful when copying examples from other functions

nassimberrada · 2023-08-06T20:47:54Z

ivy/data_classes/container/experimental/statistical.py

+        --------
+        >>> a = ivy.Container(x=ivy.array([[1, 2], [3, ivy.nan]]),\
+                                y=ivy.array([[ivy.nan, 1, 2], [1, 2, 3]])
+        >>> ivy.Container.static_moveaxis(a)


Likewise. Also this should be a.nanprod(axis, etc...)

nassimberrada · 2023-08-06T20:54:02Z

ivy/functional/ivy/experimental/statistical.py

+        The product of array elements over a given axis treating
+        Not a Numbers (NaNs) as ones
+
+    Functional Examples


Could you include more examples that have variations of the other arguments (axis, keepdims etc...) for comprehensiveness ?

nassimberrada · 2023-08-06T20:58:01Z

ivy_tests/test_ivy/test_functional/test_experimental/test_core/test_statistical.py

+):
+    input_dtype, x, axis, castable_dtype = dtype_x_axis_castable
+    x = x[0]
+    # Added clipping because the min_value and max_value arguments


This works but it would be good if the helper function could be properly used to set the necessary boundaries

nassimberrada · 2023-08-06T21:00:29Z

ivy_tests/test_ivy/test_functional/test_experimental/test_core/test_statistical.py

+    x = x[0]
+    # Added clipping because the min_value and max_value arguments
+    # don't seem to clip the returned array.
+    # Also, for really big/small numbers some backends return something like Xe300


We shouldn't be avoiding these corner cases, instead the output should remain uniform across backends. If different backends return different outputs for the same input values then I suggest you take the majority output as your target and modify the implementation of the backends that don't provide this output such that they do.

nassimberrada · 2023-08-06T21:01:23Z

ivy_tests/test_ivy/test_functional/test_experimental/test_core/test_statistical.py

+                and on_device == "cpu"
+            )
+        )
+    # This assumption is made because for some reason when


Likewise here, we don't want to just avoid testing for these cases. Again, I suggest you make all backend implementations converge to the majority output dtype

BebehCodes · 2023-08-17T17:08:19Z

Hey @nassimberrada , sorry for the delayed response, I didn't have access to my workspace. I believe I took care of all your comments. Have a look and tell me what you think. Thank you!

nassimberrada · 2023-08-17T19:33:46Z

Hey @BebehCodes, thanks for making those changes. Looks okay I think, the only issue left to address is the paddle backend which seems to fail, you can get more details by going to display_test_results -> Combined Test Results, and searching for the Falsifying Example for your function in the paddle backend.

Also, I noticed you modified the to_numpy function in the paddle general module, could you explain why you needed to ?

BebehCodes · 2023-08-19T10:32:22Z

Hey @nassimberrada , thanks for your feedback. I think paddle is fixed now.

Regarding to_numpy, in some cases I noticed that a paddle bfloat16 tensor was converted to a uint16 array and my test cases would fail due to different returned types.

Have a look and tell me what you think!

nassimberrada · 2023-08-21T11:21:10Z

Hey @BebehCodes, looks mostly good. Regarding the modified numpy function I'm afraid we can't pull this given numpy doesn't support bfloat16, I would suggest you try to tackle the dtype inconsistency by casting the incorrect dtype into the expected one as and when needed, if that makes sense. Could you revert your additions to the to_numpy function and try to address the failed cases differently ? Thanks!

# Conflicts: # ivy_tests/test_ivy/helpers/function_testing.py

BebehCodes · 2023-08-26T14:02:44Z

Hey @nassimberrada , I've tried handling it by either doing casting when this occurs while testing, etc, but the thing is that the wrong casting that happens from to_numpy also changes the final results due to the different types, so I can handle the different dtypes but I can't seem to be able to do something about the numerical results. I noticed that something similar happens to torch's to_numpy where they also handle the bfloat16 type exclusively, similarly to how I did it for paddle. Is it possible that this could be the correct solution?
Thank you!

nassimberrada · 2023-08-29T22:59:39Z

Hey, sorry for the delayed feedback. I understand, but are we sure the errors are caused by data type discrepancies ? Some of the value mismatches seem to indicate there might be another issue at play, for e.g this one

 AssertionError:  the results from backend paddle and ground truth framework tensorflow do not match
    16256.0!=1.0 
   Falsifying example: test_nanprod(
       backend_fw='paddle',
       on_device='cpu',
       dtype_x_axis_castable=('bfloat16',
        [array([1], dtype=bfloat16)],
        0,
        'bfloat16'),
       keep_dims=False,
       [rest of the error]

Does the fix you added to _to_numpy() solve these issues ? Also I see you're making changes to a function in the helper functions modules, is this for testing purposes ? Ideally we would want to contain function-specific testing within the function's test, but maybe this is serving another purpose.
One last point, I'm not sure if i'm seeing an input generation strategy for the initial argument. If not then we should probably be accounting for it as well. Thanks!

BebehCodes · 2023-08-31T11:40:47Z

Hey @nassimberrada , sorry for the confusion, I reverted the changes of to_numpy and pushed them, so that's why you are seeing this error. This value difference seems to happen because some weird overflowing or something happens because the paddle tensor is cast to uint16 instead of keeping its bfloat16 type. With the changes in to_numpy I didn't come across something similar and seemed to fix this issue as well.

The changes I made in the function helpers were in order to handle the error since I reverted to_numpy back to its original implementation, but they didn't seem to work.

Yes you are right about initial, I will add the strategy!

Generally, I haven't figured out a way to bypass the wrong casting that happens by paddle's to_numpy other than the changes I made previously regarding the handling of bfloat16.

nassimberrada · 2023-09-01T14:12:47Z

Hey, thanks for clarifying. I think the same remark applies to your additions to the helper function, ideally we want to keep these exception handling cases contained within the function. With that said, have you tried making use of the with_unsupported_dtypes decorator ? I believe this could be warranted as you're making use of functions that have a restricted set of supported data types such as paddle.nan_to_num which only takes float32 and float64, as shown in the documentation. I suggest you take a look at the corresponding section of our docs to explore this a bit. Let me know how this goes, thanks!

…cks in __cummax due to error from lint

# Conflicts: # ivy/functional/backends/paddle/experimental/statistical.py

BebehCodes · 2023-09-05T11:12:56Z

Hey @nassimberrada . Thanks a lot for your suggestion! I added the supported dtypes decorator and the tests are passing for all backends. Also, I added the strategy for the initial argument as well.
Have a look and tell me what you think!
Thanks!

nassimberrada · 2023-09-05T13:18:54Z

Yeah, looks good to me now, great work!. I'll be merging then, thanks again for contributing!

Added nanprod to all backends, ivy array and container and also added…

7eea4f5

… unit test

ivy-leaves added Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards Ivy API Experimental Run CI for testing API experimental/New feature or request PaddlePaddle Backend Developing the Paddle Paddle Backend. Ivy Functional API labels Aug 2, 2023

ivy-seed assigned nassimberrada Aug 2, 2023

nassimberrada reviewed Aug 6, 2023

View reviewed changes

BebehCodes added 5 commits August 17, 2023 15:13

Merge remote-tracking branch 'upstream/main' into feature/nanprod

627402c

Fixed typos in container docstrings

cf5f14a

Added more examples of usage in docstrings

5dc99bc

Fix for corner cases in testing bfloat16 type

848cb3c

Fix for torch's half precision on cpu

ca0fe3b

BebehCodes requested a review from nassimberrada August 17, 2023 17:08

Fix for result inaccuracies in paddle and torch

7122c01

Removed changes in paddle to_numpy and handled case in function testing

480a1d3

BebehCodes requested a review from CatB1t as a code owner August 26, 2023 09:09

Merge remote-tracking branch 'upstream/main' into feature/nanprod

637be02

# Conflicts: # ivy_tests/test_ivy/helpers/function_testing.py

BebehCodes added 3 commits September 5, 2023 13:01

Added supported dtypes decorator for paddle. Also fixed some type che…

271c0b1

…cks in __cummax due to error from lint

Merge remote-tracking branch 'upstream/main' into feature/nanprod

946c5ce

# Conflicts: # ivy/functional/backends/paddle/experimental/statistical.py

Added initial argument and strategy

b0aef6f

nassimberrada merged commit b507163 into ivy-llc:main Sep 5, 2023
107 of 133 checks passed

druvdub pushed a commit to druvdub/ivy that referenced this pull request Oct 14, 2023

Added nanprod to ivy experimental API (ivy-llc#21198)

67ad056

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added nanprod to ivy experimental API #21198

Added nanprod to ivy experimental API #21198

BebehCodes commented Aug 2, 2023

nassimberrada left a comment

nassimberrada Aug 6, 2023

nassimberrada Aug 6, 2023

nassimberrada Aug 6, 2023

nassimberrada Aug 6, 2023

nassimberrada Aug 6, 2023

nassimberrada Aug 6, 2023

BebehCodes commented Aug 17, 2023

nassimberrada commented Aug 17, 2023

BebehCodes commented Aug 19, 2023

nassimberrada commented Aug 21, 2023 •

edited

Loading

BebehCodes commented Aug 26, 2023

nassimberrada commented Aug 29, 2023

BebehCodes commented Aug 31, 2023

nassimberrada commented Sep 1, 2023

BebehCodes commented Sep 5, 2023

nassimberrada commented Sep 5, 2023

Added nanprod to ivy experimental API #21198

Added nanprod to ivy experimental API #21198

Conversation

BebehCodes commented Aug 2, 2023

nassimberrada left a comment

Choose a reason for hiding this comment

nassimberrada Aug 6, 2023

Choose a reason for hiding this comment

nassimberrada Aug 6, 2023

Choose a reason for hiding this comment

nassimberrada Aug 6, 2023

Choose a reason for hiding this comment

nassimberrada Aug 6, 2023

Choose a reason for hiding this comment

nassimberrada Aug 6, 2023

Choose a reason for hiding this comment

nassimberrada Aug 6, 2023

Choose a reason for hiding this comment

BebehCodes commented Aug 17, 2023

nassimberrada commented Aug 17, 2023

BebehCodes commented Aug 19, 2023

nassimberrada commented Aug 21, 2023 • edited Loading

BebehCodes commented Aug 26, 2023

nassimberrada commented Aug 29, 2023

BebehCodes commented Aug 31, 2023

nassimberrada commented Sep 1, 2023

BebehCodes commented Sep 5, 2023

nassimberrada commented Sep 5, 2023

nassimberrada commented Aug 21, 2023 •

edited

Loading