-
Notifications
You must be signed in to change notification settings - Fork 574
Equivariant layers in three dimensions #1372
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
… coded SiLU Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
…ecision in equivalence tests Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
c05d958 to
4e6a338
Compare
|
/blossom-ci |
Greptile OverviewGreptile SummaryThis PR extends the symmetry module with SO(3) equivariant layers, building on the existing SO(2) foundation from PR #1367. The implementation adds three key components: Core implementations:
Key strengths:
Architecture: The code is well-structured, follows coding standards (MOD-000a placement in experimental, MOD-001 Module inheritance, MOD-003 documentation), and includes appropriate validation. The previous review comments about docstring inconsistencies and shape validation have been noted but not yet addressed in this iteration. Important Files Changed
|
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.
5 files reviewed, no comments
|
/blossom-ci |
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
|
/blossom-ci |
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
|
/blossom-ci |
|
/blossom-ci |
|
/blossom-ci |
|
@peterdsharpe @CharlelieLrt ready for review (finally)! |
|
/blossom-ci |
|
/blossom-ci |
peterdsharpe
left a comment
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.
Looks good overall! Left some (optional) comments, and we might want to revisit a few things when this module graduates to nn (e.g., can SO3 linear inherit from Linear, so that we don't have to write our own initialization? Can we generalize the SO3 and SO2 layers to be dimensionally-generic?), but no need to hold this up.
Nice work, looking forward to giving these a spin!
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
|
/blossom-ci |
Yeah let's definitely touch base when it moves out of experimental: I need to play around with composing the layers and running it on real data first, and once I've dogfood'd it a little bit might have a more informed decision to make about these things. If you get the chance, too, let me know if there are pain points |
PhysicsNeMo Pull Request
Description
This PR extends #1367 by adding 3D equivariance preserving linear transformations:
SO3LinearGridin isolation, and composed together with nonlinearities withSO3ConvolutionBlock. Accompanying these changes are also routines to compute 3D rotations, both as part of testing as well as for the actual workflow itself.The full intended workflow, now with these layers is given some set of edge-wise spherical harmonic embeddings with shape
[num_edges, lmax + 1, mmax + 1, 2, num_channels], the Wigner rotation aligns the embeddings along the direction of their edges, reduces an SO3 problem into an SO2, i.e. the hard part is done in SO21:The end result should be transformed edge embeddings, and an aggregation operation will get you the updated node embeddings.
Rotations, through the
EdgeRotationclass, can be performed at arbitrary precision as needed: for example, when training at lower precision, we can choose to perform rotations at higher precision to maintain information. Conversely, maybe the rotation could be done at lower precision without too much degradation.One potential gotcha is that the Wigner matrix is cached: the idea is that, an architecture will comprise multiple SO2/SO3 blocks, and the rotation matrix only needs to be computed once per batch. I've left notes in the docstrings to remind users to manually clear the cache after backprop/optimization steps are done as to not have dangling graphs. I've thought about writing a hash-based caching system (i.e. hash the batch, and reuse the matrix only if the hash matches), but part of my design philosophy was to minimize branching with
torch.compilein mind.The PR also adds unit tests that more or less mirror the SO2 ones (also updating them to homogenize), with a little more coverage due to the complexity of 3D operations.
Checklist
Dependencies
None
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.
Footnotes
This is to avoid the expensive tensor product ↩