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

fix: torch backend- gather fix #27757

Merged
merged 101 commits into from
Mar 23, 2024
Merged

Conversation

Kacper-W-Kozdon
Copy link
Contributor

@Kacper-W-Kozdon Kacper-W-Kozdon commented Dec 14, 2023

PR Description

Previous implementation of gather() from torch backend was incorrect but did not get caught by pytest. I corrected a vast majority of issues. Pytest might still show some errors for edge cases due to the very limited implementation of torch.gather() and various workarounds needed to recreate tensorflow's ground truth behaviour, they'll likely be related to dimensionality and I'm still removing them (it's all in the conditions for reshaping and expanding). Side note: the official tensorflow's documentation for gather contains errors in the expression to calculate the dimensions of the output.

Related Issue

Closes #26498

Checklist

  • Did you add a function?
  • Did you add the tests?
  • Did you run your tests and are your tests passing?
  • Did pre-commit not fail on any check?
  • Did you follow the steps we provided?

Socials

Kacper-W-Kozdon and others added 30 commits November 30, 2023 21:21
Co-authored-by: NripeshN <nripesh14@gmail.com>
Co-authored-by: NripeshN <nripesh14@gmail.com>
Co-authored-by: NripeshN <nripesh14@gmail.com>
Co-authored-by: Bhushan Srivastava <59949692+he11owthere@users.noreply.github.com>
…on call (ivy-llc#27428)

Co-authored-by: NripeshN <nripesh14@gmail.com>
Co-authored-by: NripeshN <nripesh14@gmail.com>
…eters when doing a to_keras_module on an ivy.Module instance
Copy link
Contributor

@Ishticode Ishticode left a comment

Choose a reason for hiding this comment

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

Hi @Kacper-W-Kozdon
Thnx for the fixes. The tests all look fine. You really have put a lot of effort into this :)
I might ask @vedpatwardhan to have a quick look before merging.
Sorry for the delays on my end. One question I still have is that your initial comment you said the torch backend implementation was wrong and pytest wasn't catching it. As long as i can see we have not broadened the test to include newer cases. We have only restricted it somewhat with safety and tolerances. So how do we know that those cases that were previously undetected are still not going unnoticed?

@Kacper-W-Kozdon
Copy link
Contributor Author

Kacper-W-Kozdon commented Feb 27, 2024

Hi @Kacper-W-Kozdon Thnx for the fixes. The tests all look fine. You really have put a lot of effort into this :) I might ask @vedpatwardhan to have a quick look before merging. Sorry for the delays on my end. One question I still have is that your initial comment you said the torch backend implementation was wrong and pytest wasn't catching it. As long as i can see we have not broadened the test to include newer cases. We have only restricted it somewhat with safety and tolerances. So how do we know that those cases that were previously undetected are still not going unnoticed?

I'm not fully sure but I think my pytest was not installed properly at that time- gather() was my first contribution, the tests were passing even though they shouldn't have, which I haven't realised at first (after I reinstalled everything, they were, indeed, throwing errors).

As for the recent errors- #28168 solved them; the tests for gather were generating values too big for their dtype, which was causing assertion errors going beyond the tolerances. So it's the combination of both of these PRs that solved the issues.

Copy link
Contributor

@vedpatwardhan vedpatwardhan left a comment

Choose a reason for hiding this comment

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

lgtm! Could you please revert the changes to the demos submodule? The PR should be good to merge once that's done @Ishticode. Thanks @Kacper-W-Kozdon 😄

@Kacper-W-Kozdon
Copy link
Contributor Author

lgtm! Could you please revert the changes to the demos submodule? The PR should be good to merge once that's done @Ishticode. Thanks @Kacper-W-Kozdon 😄

Done, docs/demos changes reverted 👍

@Kacper-W-Kozdon
Copy link
Contributor Author

@vedpatwardhan @Ishticode I added explicitly the tolerance for bfloat16- hopefully, this will prevent the gradient test tripping the tolerances assertion on this type. Other than that, it seems to be ready.

Copy link
Contributor

@vedpatwardhan vedpatwardhan left a comment

Choose a reason for hiding this comment

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

Would be worth confirming if the failures in test_paddle_gather and test_paddle_gather_nd are related to the changes made. Otherwise we're good to merge, thanks @Kacper-W-Kozdon @Ishticode 😄

@ivy-seed ivy-seed added the Stale label Mar 22, 2024
@ivy-seed
Copy link

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.

Copy link
Contributor

@Ishticode Ishticode left a comment

Choose a reason for hiding this comment

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

the two tests referenced above were failing previously already. I think its good to merge. Thank you very much @Kacper-W-Kozdon for the great effort into this contribution. Thank you very much

@Ishticode Ishticode merged commit 4539031 into ivy-llc:main Mar 23, 2024
110 of 141 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ivy Functional API PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gather