Skip to content

Conversation

frostedoyster
Copy link
Collaborator

@frostedoyster frostedoyster commented Oct 15, 2025

As explained here pytorch/pytorch#41162, apparently having too many of the same index hurts performance dramatically in the backward of this operation (that we use for message passing). At the moment, our index for padding is always 0 and this creates a lot of duplicate indices.

Before:
image

After:
image

The speed improves (on my laptop's GPU) from 252 ms to 220 ms on a system of 5184 atoms.


📚 Documentation preview 📚: https://metatrain--828.org.readthedocs.build/en/828/

@frostedoyster frostedoyster force-pushed the speed-up-pet-backward-indexing branch from ead85ba to 6998ad3 Compare October 15, 2025 04:42
@frostedoyster frostedoyster force-pushed the speed-up-pet-backward-indexing branch from 6998ad3 to ff6ef85 Compare October 15, 2025 04:43
Copy link
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

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

I'll let @abmazitov comment on how much this makes sense for PET, but the code looks good to me

@frostedoyster frostedoyster marked this pull request as ready for review October 15, 2025 11:38
@frostedoyster
Copy link
Collaborator Author

cscs-ci run

@abmazitov
Copy link
Contributor

This looks good to me, the branch is outdated so feel free to merge after updating to the latest changes in main

@frostedoyster
Copy link
Collaborator Author

cscs-ci run

@frostedoyster frostedoyster enabled auto-merge (squash) October 20, 2025 05:59
@frostedoyster frostedoyster enabled auto-merge (squash) October 20, 2025 06:04
@frostedoyster
Copy link
Collaborator Author

cscs-ci run

@frostedoyster
Copy link
Collaborator Author

cscs-ci run

@frostedoyster frostedoyster merged commit d231adb into main Oct 20, 2025
16 checks passed
@frostedoyster frostedoyster deleted the speed-up-pet-backward-indexing branch October 20, 2025 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants