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

P var #18995

Merged
merged 24 commits into from
Aug 17, 2023
Merged

P var #18995

merged 24 commits into from
Aug 17, 2023

Conversation

samthakur587
Copy link
Contributor

Close #18302

@ivy-leaves ivy-leaves added the Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist label Jul 8, 2023
Copy link
Contributor

@rishabgit rishabgit left a comment

Choose a reason for hiding this comment

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

Hi! I could be missing something obvious, but I'm not too sure why we're updating the paddle/tensor/stat.py file here? 🤔
Apart from that, could you take care of the lint error? Hope this helps - https://unify.ai/docs/ivy/overview/contributing/setting_up.html#pre-commit 😄

@samthakur587
Copy link
Contributor Author

hey @rishabgit sorry for the late reply i was busy in my acadamic work for last week. Now i am solving the lint error and merge conflict.

and i am implemented the var method first in stat file in paddle frontend because when i am try to call the ivy.var directly in tensor.py file it giving me error in testing. so i implemented that in stat first.

thank you

Copy link
Contributor

@rishabgit rishabgit left a comment

Choose a reason for hiding this comment

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

Hey @MahmoudAshraf97, sorry about tagging on a paddle issue 😥 -
paddle var supports float16
But ivy's backend implementation of var calls _std that calls paddle's mean which doesn't support float16 (docs) and throws an error during the testing.

What should be ideally done for this? I don't think removing float16 from the frontend is the way to go, and I'm sure there's some decorator that handles this case that I'm ignorant of, so thought best to cc you here 😄
PS: @samthakur587 I don't think wrapping the var through the stats file is necessary here and also noticed the supported dtyes needed an update, so I made the changes to minimize the overhead of back and forth.
In case you're aware of a workaround for the issue above, feel free to chime in since you'd have the best knowledge with the deep dive you've done for this task, thanks! 😄

@MahmoudAshraf97
Copy link
Contributor

Hey @MahmoudAshraf97, sorry about tagging on a paddle issue 😥 - paddle var supports float16 But ivy's backend implementation of var calls _std that calls paddle's mean which doesn't support float16 (docs) and throws an error during the testing.

What should be ideally done for this? I don't think removing float16 from the frontend is the way to go, and I'm sure there's some decorator that handles this case that I'm ignorant of, so thought best to cc you here 😄 PS: @samthakur587 I don't think wrapping the var through the stats file is necessary here and also noticed the supported dtyes needed an update, so I made the changes to minimize the overhead of back and forth. In case you're aware of a workaround for the issue above, feel free to chime in since you'd have the best knowledge with the deep dive you've done for this task, thanks! 😄

Hi @rishabgit , paddle.var function uses paddle.mean internally, so it can't support a dtype than mean doesn't support even if the docs state so, I just tested it and float16 is not supported for both on cpu and supported only on gpu, the unsupported dtypes for paddle functions should be decided by experiments not by the documentation and should be decided for both cpu and gpu

@samthakur587
Copy link
Contributor Author

Hey @rishabgit sorry for the late reply. i am busy last week in my academics work. and now i have seen that the paddle version is updated to 2.5.1 and i have update the version and now i am doing the local test for the function.

and as the @MahmoudAshraf97 suggested i am removed the float16 dtype. because the mean does not supported the float16.

thank you

@samthakur587
Copy link
Contributor Author

Hey @rishabgit i am testing this in local and i got stuck in this error . Is there any problem with writing the test function or the actual method 'var ' in tensor.py can you please check and let me know where is the problem.

thank you

@samthakur587
Copy link
Contributor Author

this is the s.s of the error

image

@samthakur587
Copy link
Contributor Author

and i have seen the std method for paddle frontend method in tensor.py has wrong implementation. in this method he used paddle 2.4.5 and unsported dtype float32, float64 which is only supported by _std (mean). i may crate a bug in the code. i think we should have to correct it.

image

thank you

Copy link
Contributor

@rishabgit rishabgit left a comment

Choose a reason for hiding this comment

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

Hi, you'll have to add backend_fw arg in testing fn

I'm getting the same error that you reported - ivy.utils.exceptions.IvyBackendException: numpy: nested_map: numpy: nested_map: isinstance() arg 2 must be a type, a tuple of types, or a union

I'll suggest syncing your branch with latest ivy updates since there have been a lot of changes in the testing pipeline recently, which could be the culprit here

Feel free to re-request review after making these changes, thanks! 😄

@samthakur587
Copy link
Contributor Author

hey @rishabgit i am tried to solve this error many time but still it is coming. i know the problem is in my test function but i am not getting where is this. can you please check because i try every method like in place of using the _statistical_dtype_values i used try this

dtype_x_axis=helpers.dtype_values_axis(
        available_dtypes=st.one_of(helpers.get_dtypes("float")),
        min_axis=-1,
        max_axis=0,
        min_num_dims=1,
        force_int_axis=True,
    ),

but it also giving the same error.

thank you

@rishabgit
Copy link
Contributor

Hi! Sorry about the delay, will come back to this soon!

@rishabgit
Copy link
Contributor

Tests were failing due to unrelated issues which were recently fixed. I tested your code on latest ivy and everything passes. Thanks for adding this! 😄

@rishabgit rishabgit merged commit f692587 into ivy-llc:main Aug 17, 2023
194 of 266 checks passed
@samthakur587 samthakur587 deleted the p_var branch August 17, 2023 12:28
sushmanthreddy pushed a commit to sushmanthreddy/ivy that referenced this pull request Aug 17, 2023
Co-authored-by: Rishab Mallick <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

var
4 participants