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

jax.numpy delivers ediff1d on mathematical functions #11801

Merged
merged 9 commits into from
Mar 6, 2023

Conversation

BrookeMa
Copy link
Contributor

@BrookeMa BrookeMa commented Mar 6, 2023

Hi, Because the force push caused conflicts, so I make a new PR again. Please help me to review my code. Thank you
Close #11072

@ivy-leaves 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 JAX Frontend Developing the JAX Frontend, checklist triggered by commenting add_frontend_checklist Ivy Functional API labels Mar 6, 2023
@Daniel4078
Copy link
Contributor

Daniel4078 commented Mar 6, 2023

you are supposed to add a frontend function for ediff1d of jax right? why did you add those backend experimental functions instead?

@BrookeMa
Copy link
Contributor Author

BrookeMa commented Mar 6, 2023

Hi, Daniel. Because some unit tests case didn't pass. (Can't reach 100% pass) It's related to the backend. So I did it. Can I implement the backend together or separate it into two PR?

@Daniel4078
Copy link
Contributor

well, I need to see your frontend version of ediff1d first

@Daniel4078
Copy link
Contributor

also, you can not just add functions to our experimental module, all the functions that we plan to have there is listed here: #3856

@BrookeMa
Copy link
Contributor Author

BrookeMa commented Mar 6, 2023

well, I need to see your frontend version of ediff1d first

Okay, thanks. I will delete the code related to the backend. And make a new PR. How about my frontend functions? Does it have any problems?

@Daniel4078
Copy link
Contributor

It looks find to me, but I suppose it only works with your backend code right?

@BrookeMa
Copy link
Contributor Author

BrookeMa commented Mar 6, 2023

It looks find to me, but I suppose it only works with your backend code right?

Yes, If I delete the backend functions, it will fail the current test case. If possible, can I only test the frontend functions?

@BrookeMa
Copy link
Contributor Author

BrookeMa commented Mar 6, 2023

It looks find to me, but I suppose it only works with your backend code right?

This is my test case. I refer to diff.

/Users/yema/miniconda3/envs/ivy_dev/bin/python /Applications/PyCharm.app/Contents/plugins/python/helpers/pydev/pydevd.py --multiprocess --qt-support=auto --client 127.0.0.1 --port 50981 --file /Applications/PyCharm.app/Contents/plugins/python/helpers/pycharm/_jb_pytest_runner.py --target ivy_tests/test_ivy/test_frontends/test_jax/test_jax_numpy_math.py::test_jax_numpy_ediff1d 
Testing started at 14:31 ...
Connected to pydev debugger (build 223.8617.48)
Launching pytest with arguments ivy_tests/test_ivy/test_frontends/test_jax/test_jax_numpy_math.py::test_jax_numpy_ediff1d --no-header --no-summary -q in /Users/yema/Desktop/ivy

Use Database in ReadOnly Mode with local caching !
============================= test session starts ==============================
collecting ... collected 4 items

ivy_tests/test_ivy/test_frontends/test_jax/test_jax_numpy_math.py::test_jax_numpy_ediff1d[cpu-ivy.functional.backends.numpy-False-False] 
ivy_tests/test_ivy/test_frontends/test_jax/test_jax_numpy_math.py::test_jax_numpy_ediff1d[cpu-ivy.functional.backends.jax-False-False] 
ivy_tests/test_ivy/test_frontends/test_jax/test_jax_numpy_math.py::test_jax_numpy_ediff1d[cpu-ivy.functional.backends.tensorflow-False-False] 
ivy_tests/test_ivy/test_frontends/test_jax/test_jax_numpy_math.py::test_jax_numpy_ediff1d[cpu-ivy.functional.backends.torch-False-False] 

======================== 4 passed, 1 warning in 16.24s =========================

Process finished with exit code 0
PASSED [ 25%]PASSED [ 50%]PASSED [ 75%]PASSED [100%]

@Daniel4078
Copy link
Contributor

ok, just to make sure, have you tried using the existing functions in Ivy's module directly or compositionally to achieve the desired behavior? because we might be about to skip the problem of adding a new set of backend functions and tests

@BrookeMa
Copy link
Contributor Author

BrookeMa commented Mar 6, 2023

ok, just to make sure, have you tried using the existing functions in Ivy's module directly or compositionally to achieve the desired behavior? because we might be about to skip the problem of adding a new set of backend functions and tests

Yes, I didn't use ivy's functions on my implementation function. What should I do now? Delete the backend functions and make a new PR, right?

@Daniel4078
Copy link
Contributor

Daniel4078 commented Mar 6, 2023

well you can try remove the unnecessary changes on this branch if you know how to. but you are free to create a new branch and pull request

@BrookeMa
Copy link
Contributor Author

BrookeMa commented Mar 6, 2023

well you can try remove the unnecessary changes on this branch if you know how to. but you are free to create a new branch and pull request

Great, thank you

@BrookeMa
Copy link
Contributor Author

BrookeMa commented Mar 6, 2023

well you can try remove the unnecessary changes on this branch if you know how to. but you are free to create a new branch and pull request

I have deleted four functions about the backend of ediff1d. Can you help me review it again? Is there any unnecessary code now?

Copy link
Contributor

@Daniel4078 Daniel4078 left a comment

Choose a reason for hiding this comment

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

check the comments below, thanks

ivy/array/experimental/elementwise.py Outdated Show resolved Hide resolved
@Daniel4078 Daniel4078 removed the Ivy API Experimental Run CI for testing API experimental/New feature or request label Mar 6, 2023
Copy link
Contributor

@Daniel4078 Daniel4078 left a comment

Choose a reason for hiding this comment

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

I think it is good enough. Thank you!

@Daniel4078 Daniel4078 merged commit c98b3c2 into ivy-llc:master Mar 6, 2023
@BrookeMa
Copy link
Contributor Author

BrookeMa commented Mar 6, 2023

I think it is good enough. Thank you!

Thanks for your review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards Ivy Functional API JAX Frontend Developing the JAX Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ediff1d
3 participants