fix: non-periodic systems in TorchSim graph construction#161
Merged
Conversation
The TorchSim code path (build_graph_from_simstate) was passing raw pbc=False and a zero cell for non-periodic molecules, while the ASE calculator path (_normalize_atoms) creates a large fake periodic cell and wraps positions into it. This caused the M3GNet model to receive different graph inputs for the same molecule, producing inconsistent energies and forces. Add _normalize_nonperiodic_systems() that replicates the same fake-cell normalization for the TorchSim path, ensuring both code paths feed identical inputs to the model. Fixes #160 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add unit tests for _normalize_nonperiodic_systems() verifying: - Periodic systems pass through unchanged - Non-periodic molecules get a large fake periodic cell - Cell and positions match the ASE _normalize_atoms code path - Original SimState tensors are not mutated Add end-to-end test (test_wrapper_molecule_consistency) comparing TorchSimWrapper vs MatterSimCalculator on a water molecule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielzuegner
approved these changes
May 28, 2026
Phono3py v4 defaults to compact force constants format, changing fc2 shape from (n_satom, n_satom, 3, 3) to (n_patom, n_satom, 3, 3). Explicitly pass is_compact_fc=False to produce_fc2() and produce_fc3() to maintain the full format expected by downstream code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
2 participants
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.
Summary
Fixes the TorchSim interface to produce results consistent with the ASE calculator for non-periodic (molecular) systems.
Closes #160
Problem
The ASE calculator path (
_normalize_atomsinconverter.py) handles non-periodic molecules by creating a large fake periodic cell and wrapping positions into it. The TorchSim path (build_graph_from_simstate) skipped this normalization, passingpbc=Falseand a zero cell directly to the model. This produced different graph inputs for the same molecule, causing energy and force mismatches.This was caught by torch-sim CI (job link), where all 4 molecular consistency tests failed (benzene, pentacene, aspirin, alanine dipeptide) while all 18 periodic systems passed.
Changes
src/mattersim/torchsim/graph_construction.py_normalize_nonperiodic_systems()that detects fully non-periodic systems and applies the same fake-cell + position-wrapping logic as_normalize_atomsbuild_graph_from_simstate()to call this normalization beforecreate_batch_graph_dicttests/torchsim/test_torchsim.py