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

Implemented testing for copy #19683

Merged
merged 25 commits into from
Oct 19, 2023
Merged

Implemented testing for copy #19683

merged 25 commits into from
Oct 19, 2023

Conversation

KevinUli
Copy link
Contributor

No description provided.

@KevinUli KevinUli requested a review from CatB1t as a code owner July 20, 2023 09:34
@KevinUli KevinUli removed the request for review from CatB1t July 20, 2023 12:43
@AnnaTz AnnaTz requested a review from CatB1t July 24, 2023 10:43
Copy link
Contributor

@AnnaTz AnnaTz left a comment

Choose a reason for hiding this comment

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

Hi @KevinUli, this looks good overall but you will need to merge master and resolve the conflicts. There's been a refactor in function_testing.py regarding the backend handling so make sure to keep that in mind. Thanks!

ivy_tests/test_ivy/helpers/test_parameter_flags.py Outdated Show resolved Hide resolved
ivy_tests/test_ivy/helpers/function_testing.py Outdated Show resolved Hide resolved
@@ -309,6 +314,18 @@ def test_function(
target_fn, *args, test_compile=test_flags.test_compile, **kwargs
)

if test_flags.with_copy:
array_fn = ivy.is_array
if "copy" in list(inspect.signature(target_fn).parameters.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't do this kind of search in arguments for each example, but instead this logic should be moved into the decorator where this is done only once.

Copy link
Contributor Author

@KevinUli KevinUli Aug 10, 2023

Choose a reason for hiding this comment

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

By decorator, do you mean the handle_frontend_test and handle_test decorator?

ivy_tests/test_ivy/helpers/function_testing.py Outdated Show resolved Hide resolved
@KevinUli KevinUli reopened this Aug 10, 2023
@ivy-seed ivy-seed assigned yopknopixx and unassigned xoiga123 Aug 11, 2023
@AnnaTz AnnaTz requested a review from sherry30 August 18, 2023 08:55
@KevinUli KevinUli closed this Sep 15, 2023
# Conflicts:
#	ivy_tests/test_ivy/helpers/function_testing.py
#	ivy_tests/test_ivy/helpers/test_parameter_flags.py
#	ivy_tests/test_ivy/helpers/testing_helpers.py
@KevinUli KevinUli reopened this Sep 15, 2023
@KevinUli KevinUli requested review from CatB1t and AnnaTz September 15, 2023 21:26
@KevinUli KevinUli marked this pull request as draft September 15, 2023 21:27
# Conflicts:
#	ivy_tests/test_ivy/helpers/function_testing.py
#	ivy_tests/test_ivy/helpers/test_parameter_flags.py
#	ivy_tests/test_ivy/helpers/testing_helpers.py
@KevinUli KevinUli marked this pull request as ready for review September 15, 2023 22:51
@AnnaTz
Copy link
Contributor

AnnaTz commented Sep 18, 2023

Hey @sherry30, could you review this? We have dragged this for 2 months now 😅 And I would say it's an important feature missing from the pipeline.

@ivy-seed ivy-seed assigned jieunboy0516 and unassigned yopknopixx Sep 19, 2023
Copy link
Contributor

@sherry30 sherry30 left a comment

Choose a reason for hiding this comment

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

The changes mostly looks good.
Can you add this flag to the functions which require it.

@ivy-seed
Copy link

ivy-seed commented Oct 9, 2023

This PR has been labelled as stale because it has been inactive for more than 7 days. If you would like to continue working on this PR, then please add another comment or this PR will be closed in 7 days.

@ivy-seed ivy-seed added the Stale 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!

@KevinUli KevinUli requested a review from sherry30 October 18, 2023 06:21
Copy link
Contributor

@AnnaTz AnnaTz left a comment

Choose a reason for hiding this comment

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

Hi @KevinUli, apologies for this PR taking so long to merge. I will review it so we can finish it asap.

ivy_tests/test_ivy/helpers/function_testing.py Outdated Show resolved Hide resolved
ivy_tests/test_ivy/helpers/function_testing.py Outdated Show resolved Hide resolved
ivy_tests/test_ivy/helpers/function_testing.py Outdated Show resolved Hide resolved
@KevinUli KevinUli requested a review from AnnaTz October 18, 2023 16:29
@KevinUli
Copy link
Contributor Author

I have made the changes requested. I also updated the return call for the ivy API tests to include test_trace flag and precision_mode flag.

Copy link
Contributor

@AnnaTz AnnaTz left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@AnnaTz AnnaTz merged commit 98d5315 into ivy-llc:main Oct 19, 2023
254 of 269 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants