Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add increment method to Paddle frontend #21369

Closed
wants to merge 4 commits into from
Closed

Add increment method to Paddle frontend #21369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff8f793
add increment method
FelixHuyghe Aug 6, 2023
abfd76b
Update test_math.py fix casing
FelixHuyghe Aug 6, 2023
40e81a4
add testing, test not working
FelixHuyghe Aug 30, 2023
89ae691
add copy and datatype matching
FelixHuyghe Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ivy/functional/frontends/paddle/tensor/math.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# global

import ivy
from ivy.func_wrapper import with_unsupported_dtypes, with_supported_dtypes
from ivy.functional.frontends.paddle.func_wrapper import to_ivy_arrays_and_back
Expand Down Expand Up @@ -412,3 +413,16 @@ def any(x, axis=None, keepdim=False, name=None):
@to_ivy_arrays_and_back
def diff(x, n=1, axis=-1, prepend=None, append=None, name=None):
return ivy.diff(x, n=n, axis=axis, prepend=prepend, append=append)


@with_supported_dtypes(
{"2.5.1 and below": ("float32", "float64", "int32", "int64")}, "paddle"
)
@to_ivy_arrays_and_back
def increment(x, value=1.0):
if x.size == 1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x.size returns a list. Maybe you forgot to take its length?

Copy link
Author

@FelixHuyghe FelixHuyghe Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused by this: if I just type in the terminal something like a = ivy.array([3,4,5,6,7] and then do a.size I just get an int 5, which is what I would expect. I want to check if the whole array contains exactly one element.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FelixHuyghe Why is this if condition needed anyways?

Copy link
Author

@FelixHuyghe FelixHuyghe Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The paddle function accepts only inputs with one element:
paddle.increment(x, value=1.0, name=None) The API is usually used for control flow to increment the data of x by an amount value. Notice that the number of elements in x must be equal to 1.
The problem is that ivy.increment_incplace accepts inputs with any shape, so I thought I check if the size is equal to one and if not I return an exception. However if I try to test this by setting the max_num_size parameter for x to something larger then 1, the tests fail and I get the exception I wrote myself. I was trying to test that when you provide an input array with a size larger than 1 the function indeed raises an exception, just like the original Paddle function. But I don't think I am doing it in the correct way, if you could help me :)
I was also wondering if in this case the frontend function should return the exact same exception as when paddle.increment is called directly. This is why in my previous commit I tried calling paddle.increment directly from the frontend, to get the exact same exception, but I understand this is not the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The paddle function accepts only inputs with one element: paddle.increment(x, value=1.0, name=None) The API is usually used for control flow to increment the data of x by an amount value. Notice that the number of elements in x must be equal to 1. The problem is that ivy.increment_incplace accepts inputs with any shape, so I thought I check if the size is equal to one and if not I return an exception. However if I try to test this by setting the max_num_size parameter for x to something larger then 1, the tests fail and I get the exception I wrote myself. I was trying to test that when you provide an input array with a size larger than 1 the function indeed raises an exception, just like the original Paddle function. But I don't think I am doing it in the correct way, if you could help me :) I was also wondering if in this case the frontend function should return the exact same exception as when paddle.increment is called directly. This is why in my previous commit I tried calling paddle.increment directly from the frontend, to get the exact same exception, but I understand this is not the way to go.

@FelixHuyghe You don't need to test for values that you already know don't fall into the acceptable range of values for this function. The unit test is required to provide complete coverage of an acceptable range of values and so only values that the original function should run fine on should be generated with the unit tests if that makes sense. :)

return ivy.inplace_increment(ivy.copy_array(x), value)
else:
return ValueError(
"(InvalidArgument) The number of elements in Input(X) should be 1."
)
48 changes: 47 additions & 1 deletion ivy_tests/test_ivy/test_frontends/test_paddle/test_tensor/test_math.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# global

from hypothesis import strategies as st

import ivy

# local
import ivy_tests.test_ivy.helpers as helpers
from ivy_tests.test_ivy.helpers import handle_frontend_test
Expand Down Expand Up @@ -951,7 +954,7 @@ def test_paddle_lgamma(
test_flags=test_flags,
fn_tree=fn_tree,
on_device=on_device,
atol=1E-4,
atol=1e-4,
x=x[0],
)

Expand Down Expand Up @@ -1864,3 +1867,46 @@ def test_paddle_diff(
prepend=prepend[0],
append=append[0],
)


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't generate any input strategies for the test using the @handle_frontend_test decorator. Kindly add these and run the test using pytest to see if it passes with all backends or not

Copy link
Author

@FelixHuyghe FelixHuyghe Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I did not really understand how the testing worked indeed. In my latest commit I have added a @handle_frontend_test but my tests are not working.
For example with input data [0] and value 1, I get the error that my solution and the groundtruth solution not match: [1]!=[2]
After inspecting the testing method, I think that this is caused because the same array is passed to my implementation of the frontend function first and then the ground truth version later. The problem is that a side effect of the padddle.increment value is that the data that you give it is incremented itself. So when the frontend is called with x=[0] and value=1, the correct result [1] is returned, but causes x=[1] which is why when the groundtruth value is calculated afterwards it adds 1 to 1 and that is how the ground truth value ends up being wrong. If I manually change value to 2 for example I get the error [2]!=[4] in the same case (the one produced by the reproduce failure in my code), so it just does the addition twice for the ground truth value. I hope my explanation was understandable, I am a bit lost in how to properly test this method, so hope you can help me!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FelixHuyghe Try copying the array using something like ivy.copy_array before passing it to ivy.inplace_increment. See if that works for you and let me know :)

Copy link
Author

@FelixHuyghe FelixHuyghe Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that works! I copied the array before passing it to ivy.inplace_increment and now my tests pass (when x is of shape [1] anyways)! However, I think this implementation does not fully reflect the original paddle one, because if you pass an array to the paddle function, a side effect is that array is incremented "in place" and with my frontend implementation when the function returns the provided array is still the same (because of the copy).

Copy link
Contributor

@hmahmood24 hmahmood24 Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that works! I copied the array before passing it to ivy.inplace_increment and now my tests pass (when x is of shape [1] anyways)! However, I think this implementation does not fully reflect the original paddle one, because if you pass an array to the paddle function, a side effect is that array is incremented "in place" and with my frontend implementation when the function returns the provided array is still the same (because of the copy).

I see what you mean. I think the test for ivy.inplace_increment should give you an idea of how to test this frontend function because it is kind of similar to what's happening with ivy.inplace_increment I would think so. Although there are plenty of functions in our frontends that perform updates inplace (functions with names ending at an underscore "_"), so not sure why we need some special handling over here?

# increment
@handle_frontend_test(
fn_tree="paddle.tensor.math.increment",
dtype_and_x=helpers.dtype_and_values(
available_dtypes=st.shared(helpers.get_dtypes("valid"), key="dtype"),
min_num_dims=1,
max_num_dims=1,
min_dim_size=1,
max_dim_size=1,
),
dtype_and_value=helpers.dtype_and_values(
available_dtypes=st.shared(helpers.get_dtypes("float"), key="dtype"),
min_num_dims=1,
min_dim_size=1,
max_num_dims=1,
),
)
def test_paddle_increment(
*,
dtype_and_x,
dtype_and_value,
frontend,
test_flags,
fn_tree,
backend_fw,
):
input_dtypes, x = dtype_and_x
value_dtypes, value = dtype_and_value
# Match the data type of x and value to sum them up
x[0], value[0] = ivy.promote_types_of_inputs(x[0], value[0])
value_first_element = value[0].item(0)

helpers.test_frontend_function(
input_dtypes=[ivy.dtype(x[0]), ivy.dtype(value[0])],
backend_to_test=backend_fw,
frontend=frontend,
test_flags=test_flags,
fn_tree=fn_tree,
x=x[0],
value=value_first_element,
)