Review and Optimize SymmetrizedModel on top of #119 - #289
Merged
ppegolo merged 4 commits intoJul 15, 2026
Merged
Conversation
Contributor
|
@Luthaf don't worry about this one, we'll get to a decent final state and then request review |
Make routing, improper parity, pole recovery, dtype/device handling, and all ten documented component axes explicit and tested. Construct real Wigner matrices directly and lazily, batch-validate internally generated random transformations, and enforce canonical Cartesian and spherical bases even for empty blocks. These changes preserve already-correct cases while removing repeated packing and per-matrix synchronization.
Preserve complete output schemas, reject gradients that would otherwise be lost, support raw and exported models plus selected atoms, and define conservative derivatives consistently with metatrain. Detached energies warn and return zeros; 3D stress is limited to finite-volume full PBC. Use reference-centered float64 moments, actual returned ranks, batched geometry transforms, trusted quadrature operations, and bounded reusable Wigner storage. Build CUDA Wigner batches on CPU and transfer completed stacks to avoid per-rotation synchronization. The public guide states the finite-grid, unit, derivative, memory, and convergence contracts.
Rotate every supported requested polar-vector input and reject unsupported axial-vector inversion. Propagate requested TensorMaps, watch requested ASE state for cache invalidation, and retain only closed space-group actions that preserve model inputs. Stream centered rotational moments by batch, skip scalar no-op group discovery, preserve caller-selected periodic images and translation-distinct permutations, and accelerate large periodic site matching with conservative indexed candidates plus exact bounded fallback. Stress follows the same full-PBC finite-volume contract as SymmetrizedModel.
MichelangeloDomina
force-pushed
the
review/symmetrized-model-pr119-final-series
branch
from
July 15, 2026 15:16
79d4581 to
9f4932c
Compare
ppegolo
approved these changes
Jul 15, 2026
ppegolo
marked this pull request as ready for review
July 15, 2026 15:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a stacked follow-up to #119. It intentionally targets the
token_rotationsbranch at31cc945, so this pull request contains only thefour follow-up commits rather than repeating the complete parent PR.
Scope
The changes are limited to:
metatomic.torch.SymmetrizedModel;SymmetrizedCalculator, including the requested-input andspace-group paths needed to construct its symmetry orbit.
No metatrain model, training procedure, loss, dataset, optimizer, or base-model
architecture is modified.
Motivation
The implementation in #119 establishes the main feature, but several
mathematical, numerical, API, and scalability contracts required additional
work before the feature could be considered internally consistent and suitable
for larger grids and datasets.
The follow-up therefore:
Intentional behavioral corrections
Some results intentionally differ from the original #119 implementation:
o3_lambdaando3_sigmakeys;autograd when requested;
energies;
CPU or CUDA behavior; and
translation-distinct atom permutations.
These cases are tested against analytical, structural, or API targets.
Performance and scalability
The optimization are limited to:
number of quadrature operations; and
verification and an exact bounded fallback.
Controlled benchmarks showed material improvements on the affected paths.
Change composition
Relative to #119, this series changes 20 files:
The total increase relative to #119 is 4,471 net lines. Tests account for
55.3% of this increase. Compared with the previous draft, the runtime
implementation is unchanged while repetitive tests and documentation were
consolidated, reducing the series by 221 net lines.
Validation
Validation on the exact four-commit tip:
and 94% including branches;
git diff --check: passed;The three commits were also tested independently to preserve bisectability.
Review structure
The commits are intentionally divided into four review units:
O(3) transformation correctness and Wigner construction;
SymmetrizedModelnumerical, derivative, schema, cache, and performance contracts;ASE requested-state preservation, streaming reduction, and space-group projection;
reviewer-facing mathematical/API documentation and the direct Wigner-cache
mutation regression.
I have refreshed the draft series after a reviewability pass. The runtime implementation is unchanged from the previous draft. Repetitive tests were consolidated, non-obvious mathematical and API contracts were documented, and a direct quadrature-grid mutation regression was restored.