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.

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

feat(paddle): Add broadcast_shape function to Paddle Frontend #23574

Merged
merged 20 commits into from
Oct 16, 2023

Conversation

maxxies
Copy link
Contributor

@maxxies maxxies commented Sep 14, 2023

PR Description

Related Issue

Close #23555

Checklist

  • Did you add a function?
  • Did you add the tests?
  • Did you follow the steps we provided?

Socials:

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks Passed!

@maxxies maxxies changed the title Add broadcast_shape function to paddle frontends feat(paddle): Add broadcast_shape function to Paddle Frontend Sep 14, 2023
@ivy-leaves ivy-leaves added the Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist label Sep 14, 2023
@a0m0rajab a0m0rajab modified the milestone: #10388 Sep 14, 2023
@maxxies
Copy link
Contributor Author

maxxies commented Sep 25, 2023

@juliagsy can you have a quick review on the changes made? I am about to be removed from the application process due to slow progress.

Copy link
Contributor

@KareemMAX KareemMAX left a comment

Choose a reason for hiding this comment

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

Hi, seems like you have changed docs/demos which you might have done by mistake. Can you revert that change because we can't merge with that change in place?

@charlie572
Copy link

This is a duplicate of #22977.

Really, before I stated with this task, it had not been converted to an issue in the Add mathematical functions to Paddle Frontend issue list. So I converted it to an issue as stated in the Ivy docs. What do you reckon I do? @charlie572 @juliagsy

I don't think the scripts were working when I created my issue, so it never updated the checklist. The scripts seem to work now.

@juliagsy
Copy link
Contributor

@charlie572 thanks for flagging! seems like a glitch in the automation because the linked issue in the todo is the one from @maxxies
As it's an issue on our side, we'll try to accept both PRs and incorporate both changes, the PRs may be proceed as usual :)

@Ivanseven Ivanseven mentioned this pull request Oct 2, 2023
5 tasks
Copy link
Contributor

@juliagsy juliagsy left a comment

Choose a reason for hiding this comment

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

Hey1 The tests are still not passing unfortunately, I've double check the docs and apparently the variable names are diff, they have to be identical to native. Could you please try the below suggested change and continue working on getting the tests to pass? Thanks!

ivy/functional/frontends/paddle/math.py Outdated Show resolved Hide resolved
ivy/functional/frontends/paddle/math.py Outdated Show resolved Hide resolved
ivy_tests/test_ivy/test_frontends/test_paddle/test_math.py Outdated Show resolved Hide resolved
ivy_tests/test_ivy/test_frontends/test_paddle/test_math.py Outdated Show resolved Hide resolved
ivy_tests/test_ivy/test_frontends/test_paddle/test_math.py Outdated Show resolved Hide resolved
@maxxies
Copy link
Contributor Author

maxxies commented Oct 2, 2023

Hey1 The tests are still not passing unfortunately, I've double check the docs and apparently the variable names are diff, they have to be identical to native. Could you please try the below suggested change and continue working on getting the tests to pass? Thanks!

This is the error I still get:

 +---------------- 2 ----------------
    | Traceback (most recent call last):
    |   File "/workspaces/ivy/ivy_tests/test_ivy/test_frontends/test_paddle/test_math.py", line 493, in test_paddle_broadcast_shape
    |     helpers.test_frontend_function(
    |   File "/workspaces/ivy/ivy_tests/test_ivy/helpers/function_testing.py", line 981, in test_frontend_function
    |     value_test(
    |   File "/workspaces/ivy/ivy_tests/test_ivy/helpers/assertions.py", line 141, in value_test
    |     assert len(ret_np_flat) == len(ret_np_from_gt_flat), (
    | AssertionError: The length of results from backend paddle and ground truth framework paddle does not match
    | 
    | len(ret_np_flat) != len(ret_np_from_gt_flat):
    | 
    | ret_np_flat:
    | 
    | [array(1)]
    | 
    | ret_np_from_gt_flat:
    | 
    | []
    | Falsifying example: test_paddle_broadcast_shape(
    |     # The test always failed when commented parts were varied together.
    |     on_device='cpu',
    |     frontend='paddle',
    |     backend_fw='paddle',
    |     input_shapes_x=BroadcastableShapes(input_shapes=((1,), (1,)), result_shape=(1,)),  # or any other generated value
    |     fn_tree='ivy.functional.frontends.paddle.broadcast_shape',
    |     test_flags=FrontendFunctionTestFlags(
    |         num_positional_args=0,
    |         with_out=False,
    |         inplace=False,
    |         as_variable=[False],
    |         native_arrays=[False],
    |         test_compile=False,
    |         generate_frontend_arrays=False,
    |         transpile=False,
    |         precision_mode=False,
    |     ),  # or any other generated value
    | )
    | 
    | You can reproduce this example by temporarily adding @reproduce_failure('6.87.0', b'AXicY2BAAQAADwAB') as a decorator on your test case

@maxxies
Copy link
Contributor Author

maxxies commented Oct 2, 2023

ret_np_from_gt_flat

This ret_np_from_gt_flat returns an empty array

@maxxies maxxies requested a review from juliagsy October 2, 2023 12:37
@juliagsy
Copy link
Contributor

juliagsy commented Oct 2, 2023

Could you please try adding test_values=False? then check the values manually? Might be because they are tuples or lists instead of arrays. Some references using test_values=False: test_jax_cholesky, test_jax_eig

@maxxies
Copy link
Contributor Author

maxxies commented Oct 2, 2023

Could you please try adding test_values=False? then check the values manually? Might be because they are tuples or lists instead of arrays. Some references using test_values=False: test_jax_cholesky, test_jax_eig

I think the tuples are handled appropriately but the assertions still fail:

` kwargs["out"] = out
if is_ret_tuple:
if test_flags.generate_frontend_arrays:
flatten_ret = flatten_frontend(
ret=ret,
backend=backend_to_test,
frontend_array_fn=create_frontend_array,
)
flatten_out = flatten_frontend(
ret=out,
backend=backend_to_test,
frontend_array_fn=create_frontend_array,
)
else:
flatten_ret = flatten(backend=backend_to_test, ret=ret)
flatten_out = flatten(backend=backend_to_test, ret=out)
for ret_array, out_array in zip(flatten_ret, flatten_out):
if ivy_backend.native_inplace_support and not any(
(ivy_backend.isscalar(ret), ivy_backend.isscalar(out))
):
if test_flags.generate_frontend_arrays:
assert ret_array.ivy_array.data is out_array.ivy_array.data
else:
assert ret_array.data is out_array.data
assert ret_array is out_array
E AssertionError
E Falsifying example: test_paddle_broadcast_shape(
E on_device='cpu',
E frontend='paddle',
E backend_fw='paddle',
E input_shapes_x=BroadcastableShapes(input_shapes=((1,), (1,)), result_shape=(1,)), # or any other generated value
E test_flags=FrontendFunctionTestFlags(
E num_positional_args=0,
E with_out=True,
E inplace=False,
E as_variable=[False],
E native_arrays=[False],
E test_compile=False,
E generate_frontend_arrays=True,
E transpile=False,
E precision_mode=False,
E ),
E fn_tree='ivy.functional.frontends.paddle.broadcast_shape',
E )
E Explanation:
E These lines were always and only run by failing examples:
E /workspaces/ivy/ivy_tests/test_ivy/helpers/function_testing.py:772
E /workspaces/ivy/ivy_tests/test_ivy/helpers/function_testing.py:775
E /workspaces/ivy/ivy_tests/test_ivy/helpers/function_testing.py:778
E /workspaces/ivy/ivy_tests/test_ivy/helpers/function_testing.py:779
E /workspaces/ivy/ivy_tests/test_ivy/helpers/function_testing.py:809
E /workspaces/ivy/ivy_tests/test_ivy/helpers/function_testing.py:814
E
E You can reproduce this example by temporarily adding @reproduce_failure('6.87.0', b'AXicY2CAAUYQAgAAFgAD') as a decorator on your test case

ivy_tests/test_ivy/helpers/function_testing.py:830: AssertionError`

P.S:

It passes for Jax

@ivy-leaves ivy-leaves added the PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist label Oct 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

Thank you for this PR, here is the CI results:


This pull request does not result in any additional test failures. Congratulations!

@juliagsy
Copy link
Contributor

Hey! Apologies for the delay again! I've been looking into the code and the related function to see where I'm able to help but haven't seen something too off yet. I'll also cc some engineers from the frontends/testing team to see if they have an idea on the issue about gt returning empty list/results. (cc @AnnaTz @sherry30) thanks all!

@maxxies
Copy link
Contributor Author

maxxies commented Oct 11, 2023

Hey! Apologies for the delay again! I've been looking into the code and the related function to see where I'm able to help but haven't seen something too off yet. I'll also cc some engineers from the frontends/testing team to see if they have an idea on the issue about gt returning empty list/results. (cc @AnnaTz @sherry30) thanks all!

Sure, thanks.

@AnnaTz
Copy link
Contributor

AnnaTz commented Oct 16, 2023

Hi @juliagsy @maxxies, there were some bugs in the frontend testing pipeline which I have now fixed. The test is passing locally, so I would say we can merge as soon as the changes to the demos folder have been reverted.

Copy link
Contributor

@juliagsy juliagsy left a comment

Choose a reason for hiding this comment

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

Hey! Just a quick one, could you quickly go into docs/demos and do git checkout aaef6cb then cd back to ivy root and do git add docs/demos then commit to your own branch? There's a diff showing in docs/demos in the PR and we don't want to commit any changes in there yet, thanks!

Copy link
Contributor

@juliagsy juliagsy left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

@juliagsy juliagsy merged commit 095f4be into ivy-llc:main Oct 16, 2023
133 of 136 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

broadcast_shape
8 participants