Refactor ivy.clip function

[NripeshN]

No description provided.

[github-actions]

Thanks for contributing to Ivy! 😊👏
Here are some of the important points from our Contributing Guidelines 📝:
1. Feel free to ignore the run_tests (1), run_tests (2), … jobs, and only look at the display_test_results job. 👀 It contains the following two sections:
- Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
- New Failures Introduced: This lists the tests that are passing on main, but fail on the PR Fork. Please try to make sure that there are no such tests. 💪
2. The lint / Check formatting / check-formatting tests check for the formatting of your code. 📜 If it fails, please check the exact error message in the logs and fix the same. ⚠️🔧
3. Finally, the test-docstrings / run-docstring-tests check for the changes made in docstrings of the functions. This may be skipped, as well. 📚
Happy coding! 🎉👨‍💻

[hello-fri-end]

Hey @NripeshN ! Thanks for working on this issue. The way you have tried to fix this problem is not the correct way to go about it. Since clip is a primary function, you need to make the change in all the backend implementations of clip. For example, numpy already supports one of x_min or x_max to be None, so no update is needed there but in the jax backend it's implemented using jax.where so, you should first check that both values are not Noneat the same time, like:

    if x_min is None and x_max is None:
        raise ValueError("At least one of the x_min or x_max must be provided")

And then clip the values like :

if x_max is not None:
x = jnp.where(x > x_max, x_max, x)

if x_min is not None:
return jnp.where(x < x_min, x_min, x)

You can use the same idea to update the other backend implementations as well if they don't natively support one of the arguments to be None. Finally, you should update the test of ivy.clip such that one of x_min or x_max can be None.

Feel free to request another review when you have made the changes.

[NripeshN]

ivy-gardener
✅ Ivy gardener has formatted your code.
If changes are requested, don't forget to pull your fork.

[ivy-leaves]

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 🤗

[hello-fri-end]

Could you please update the type hints in the numpy backend and update the test to allow one of x_min or x_max to be None?

[hello-fri-end]

Awesome work so far, would you please update the tests as well :)

[hello-fri-end]

ivy-gardener
✅ Ivy gardener has formatted your code.
If changes are requested, don't forget to pull your fork.

[ShreyanshBardia]

Using tf.experimental.numpy when one of the value is None, since tf.clip_by_value doesn't accept None

[ShreyanshBardia]

Hi @hello-fri-end, tensorflow differs from all other frameworks when x_min > x_max, so should we standardise this edge case by considering numpy as the standard, and override tensorflow's behaviour? Here's an example

import numpy as np
import tensorflow as tf

tf.clip_by_value(tf.constant([1,10,13]),12,11)

np.clip([1,10,13],12,11)

[hello-fri-end]

Hi @hello-fri-end, tensorflow differs from all other frameworks when x_min > x_max, so should we standardise this edge case by considering numpy as the standard, and override tensorflow's behaviour? Here's an example

import numpy as np
import tensorflow as tf

tf.clip_by_value(tf.constant([1,10,13]),12,11)

np.clip([1,10,13],12,11)

Hey @ShreyanshBardia ! Thanks for pointing this out 🚀 It seems that torch.clamp also has the same behavior i.e it clips using the min value first and then by the max value:

import numpy as np
import tensorflow as tf
import torch

print(tf.clip_by_value(tf.constant([1,10,13]),12,11))

print(np.clip([1,10,13],12,11))

print(torch.clamp(torch.tensor([1, 10, 13]), min=12, max=11))

>>>tf.Tensor([12 12 12], shape=(3,), dtype=int32)
>>>[11 11 11]
>>>tensor([11, 11, 11])

Would you please update our implementations to match this behavior? @NripeshN In the tensorflow backend implementation, we can use tf.experimental.numpy.clip for all the cases while for jax and paddle, where the function is implemented compositionally, we should clip using x_min first and then by x_max. You should also update the test where you'd need to remove the condition that x_max always needs to greater than x_min. Feel free to request another review when you are done. Happy coding 👍

[hello-fri-end]

Hey @NripeshN ! Awesome work so far, just some minor comments this time. Will be happy to merge once these are resolved :)

[hello-fri-end]

I'm not sure if I follow why we need an else case here since x_min is not None and x_max is not None and x_min > x_max case (i.e not cond) can be handled by tf.experimental.numpy.clip as well, right? Maybe I'm missing something here..

Also, it looks like the frontend tests are failing because of cond with the following error:

E     ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

This is because x_min and x_max can also be arrays.

[ShreyanshBardia]

Oh right, sorry missed the fact that they could be arrays too, I think .any should be able to resolve this. tf.experimental.numpy.clip also follows tf.clip_by_value behaviour when x_min>x_max, which we discussed in the above comments. I think the only difference is numpy namespace and the fact that it can accept None. You can verify this from the following code.

>>> print(tf.clip_by_value(tf.constant([1,10,13]),12,11))
tf.Tensor([12 12 12], shape=(3,), dtype=int32)
>>> print(tf.experimental.numpy.clip(tf.constant([1,10,13]),12,11))
tf.Tensor([12 12 12], shape=(3,), dtype=int32)

[hello-fri-end]

Oh right, sorry missed the fact that they could be arrays too, I think .any should be able to resolve this. tf.experimental.numpy.clip also follows tf.clip_by_value behaviour when x_min>x_max, which we discussed in the above comments. I think the only difference is numpy namespace and the fact that it can accept None. You can verify this from the following code.

>>> print(tf.clip_by_value(tf.constant([1,10,13]),12,11))
tf.Tensor([12 12 12], shape=(3,), dtype=int32)
>>> print(tf.experimental.numpy.clip(tf.constant([1,10,13]),12,11))
tf.Tensor([12 12 12], shape=(3,), dtype=int32)

Ah, that's interesting. I expected tf.experimental.numpy.clip to behave exactly the same as np.clip.

[hello-fri-end]

Can we update min_val and max_val such that these can be arrays as well? They can be of any shape that is broadcastable to the shape of the input x. You should use the broadcast helper functions in the Array helpers section here --> :https://unify.ai/docs/ivy/docs/helpers/ivy_tests.test_ivy.helpers.hypothesis_helpers/ivy_tests.test_ivy.helpers.hypothesis_helpers.array_helpers.html#ivy_tests.test_ivy.helpers.hypothesis_helpers.array_helpers.mutually_broadcastable_shapes

[hello-fri-end]

Just left a comment that will fix your failing numpy test. Another thing to look into is the following error from the frontend tests:

    promoted_type = ivy.as_native_dtype(ivy.promote_types(x.dtype, x_min.dtype))
E   AttributeError: 'float' object has no attribute 'dtype'

Since x_min and x_max can also be Numbers, they won't necessarily have the dtype attribute. Would you please update all the implementations to check if this attribute is present first and then try to access it.

[NripeshN]

ivy-gardener
✅ Ivy gardener has formatted your code.
If changes are requested, don't forget to pull your fork.

[NripeshN]

ivy-gardener
✅ Ivy gardener has formatted your code.
If changes are requested, don't forget to pull your fork.

[hello-fri-end]

Looks like the failures in the CI are unrelated to this function now. Feel free to merge it - thanks @NripeshN @ShreyanshBardia 🚀