-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: manual dtype casting removal #22700
feat: manual dtype casting removal #22700
Conversation
…linear_algebra.py
Thanks for contributing to Ivy! 😊👏 |
If you are working on an open task, please edit the PR description to link to the issue you've created. For more information, please check ToDo List Issues Guide. Thank you 🤗 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Madjid-CH, thanks for looking into this, just requested a couple of changes, but they also apply to all the other functions if applicable 😄
@with_unsupported_device_and_dtypes( | ||
{"2.5.1 and below": {"cpu": ("bfloat16",)}}, backend_version | ||
@with_supported_dtypes( | ||
{"2.5.1 and below": ("float16", "uint16", "float32", "float64", "complex")}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of float16
, float32
and float64
you could just use float
, as all float dtypes are supported.
Also, I think uint16
isn't supported in paddle
Same applies to other similar instances in the paddle
backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I looked into the functions source code to figure out the supported types, for example: here I think is where the supported types checks is performed and uint16 is mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it seems like they might've started supporting uint16
in the later versions, @MahmoudAshraf97 what do you think? Thanks!
…g-removal # Conflicts: # ivy/functional/backends/paddle/experimental/statistical.py # ivy/functional/backends/paddle/general.py
understood |
@MahmoudAshraf97 can you please explain how to differentiate between a manual cast for adding support and a manual cast for backends compatibility because my approach is to test the real backend function against all available dtypes and see which dtypes behave correctly (same input same ouput dtype, not throwing error, ...) |
there is no solid diffrentiation between the two types, but as I said the casting for compatibility is mostly found in paddle backend |
Yeah you're right! There are cases where we apply casting to the output to ensure consistency, could you please share some examples in those commits where this is being done? Thanks @MahmoudAshraf97 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Madjid-CH, could you please check through if there's any unresolved conversations in the previous reviews as well? Thanks 😄
sure Ved 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Let's wait for @MahmoudAshraf97 to give it another look, thanks @Madjid-CH 😄
ivy-gardener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, you should keep the dtypes decorator as is because cpu and gpu support are different in paddle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Compliance Checks
Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.
Issue Reference
In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our contributing guide and our PR template.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Compliance Checks
Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.
Issue Reference
In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our contributing guide and our PR template.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.
understood |
…g-removal # Conflicts: # ivy/functional/backends/paddle/elementwise.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Compliance Checks
Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.
Issue Reference
In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our contributing guide and our PR template.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Compliance Checks
Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.
Issue Reference
In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our contributing guide and our PR template.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.
Thank you for this PR, here is the CI results: This pull request does not result in any additional test failures. Congratulations! |
Hey @MahmoudAshraf97, could you please review this PR so that we can get it merged soon? Thanks 😄 |
…g-removal # Conflicts: # ivy/functional/backends/paddle/activations.py # ivy/functional/backends/paddle/elementwise.py
…to manual-dtype-casting-removal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Compliance Checks
Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.
Issue Reference
In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our contributing guide and our PR template.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.
PR Description
This PR is for this task
I started with the Paddle backend. I need some feedback on my work so I can continue with the other backends.