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

EHN : ptp methos is add in Jax devicearray.py #19049

Merged
merged 22 commits into from
Aug 8, 2023
Merged

EHN : ptp methos is add in Jax devicearray.py #19049

merged 22 commits into from
Aug 8, 2023

Conversation

MuhammadNizamani
Copy link
Contributor

closed #19048

@ivy-leaves ivy-leaves added the JAX Frontend Developing the JAX Frontend, checklist triggered by commenting add_frontend_checklist label Jul 9, 2023
@MuhammadNizamani
Copy link
Contributor Author

@D0m-inic review this please

@D0m-inic
Copy link
Contributor

Hi Muhammad, looks good to me. It looks to me like the dtype and axis values for the test can be obtained from already existing helper methods, could you explain your reasoning behind creating a new composite?

@MuhammadNizamani
Copy link
Contributor Author

I want to check for shape . @D0m-inic I can change if you want.

@D0m-inic
Copy link
Contributor

It looks to me like your st.composite seems to be quite similar in functionality to helpers.dtype_values_axis which you could use?

@MuhammadNizamani
Copy link
Contributor Author

It looks to me like your st.composite seems to be quite similar in functionality to helpers.dtype_values_axis which you could use?

@D0m-inic What should I do ? should I change my test method??

@D0m-inic
Copy link
Contributor

From what I see, you can get rid of your created @st.composite, as it is similar in functionality to the helpers.dtype_values_axis function, and in the @handle_frontend_method wrapper, when passing in dtype_and_x_axis_dtype, make a call to helpers.dtype_values_axis instead.

If you have a reason to use your custom composite that i'm overlooking, please let me know 😄

@MuhammadNizamani
Copy link
Contributor Author

@D0m-inic I have make changes that you asked for and all test are passing.

@D0m-inic
Copy link
Contributor

D0m-inic commented Aug 7, 2023

Hi Muhammad, just checked your PR and it seems some tests are failing, due to complex values types not being allowed.

I see some merge conflicts too? I know about the clone PR of this function being erroneously merged already, but @juliagsy said she already resolved the conflicts so I'm assuming its just something small :)

@MuhammadNizamani
Copy link
Contributor Author

@D0m-inic I will resolve the issue and then I will mention you server

@D0m-inic
Copy link
Contributor

D0m-inic commented Aug 7, 2023

👍

@MuhammadNizamani
Copy link
Contributor Author

All tests passed on
domnic PR

@D0m-inic
Copy link
Contributor

D0m-inic commented Aug 8, 2023

Hey Muhammad, could you remove the shape parameter from dtype_values_axis for random shapes in testing? (This would also mean adding the valid_axis=True and min_num_dims=1 parameters to prevent illegal inputs).

Also, you can change the value of helpers.get_dtpyes to be "float" instead of "numeric", and then remove the conditionals for the complex numbers.

After these changes i'm happy to merge 😃

@MuhammadNizamani
Copy link
Contributor Author

Sure thanks

@MuhammadNizamani
Copy link
Contributor Author

@D0m-inic I have made changes that you asked for

@D0m-inic
Copy link
Contributor

D0m-inic commented Aug 8, 2023

LGTM 👍

@D0m-inic D0m-inic merged commit b63e98c into ivy-llc:master Aug 8, 2023
Sarvesh-Kesharwani pushed a commit to Sarvesh-Kesharwani/ivy_Sark42 that referenced this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

ptp
4 participants