Skip to content

Split weighted pair energies equally between atoms - #635

Open
Brean0 wants to merge 2 commits into
TorchSim:mainfrom
Brean0:brian/fix-pair-atom-energies
Open

Brean0 wants to merge 2 commits into
TorchSim:mainfrom
Brean0:brian/fix-pair-atom-energies

Conversation

@Brean0

@Brean0 Brean0 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Summary

PairPotentialModel adds each weighted pair energy to the system once and to each endpoint in full. As a result, the sum of energies is twice the returned system energy, for both full and half neighbor lists.

For two isolated soft-sphere atoms at distance 0.5, with epsilon=sigma=1 and alpha=2, the system energy is 0.125 but the atom energies are [0.125, 0.125]. Allocate half of the weighted pair energy to each endpoint, giving [0.0625, 0.0625] and preserving the system total of 0.125.

Reproduction

python -m pytest -q tests/models/test_pair_potential.py -k 'isolated_pair_atom_energies or batched_atom_energies_unequal_system_sizes'

The four isolated-pair cases and four unequal-batch cases fail at the expected per-atom energy assertion with baseline accumulation and pass with the fix. Both fixtures cover full and half lists with torch_nl_n2 and the default torchsim_nl backend on CPU float64. The unequal batch has three atoms in one system and two in the other; its expected atom energies are [0.0625, 0.125, 0.0625, 0.0625, 0.0625] and its system totals are [0.25, 0.125]. Each system's atom energies must sum to its own returned total.

Validation

  • python -m pytest -q tests/models/test_pair_potential.py tests/models/test_soft_sphere.py tests/models/test_lennard_jones.py: 59 passed after adding the unequal-batch regression.
  • Repository CI command python -m pytest --cov=torch_sim --cov-report=xml: exited before collection because the prepared environment does not have pytest-cov (--cov arguments were unrecognized).
  • Final repository-wide python -m pytest: 1011 passed, 208 skipped, exit 0. Each final run includes all regression additions. Three PyTables UnclosedFileWarning messages followed the summary during trajectory-file cleanup.
  • Ruff 0.17.1 lint and format checks on both changed files: passed.
  • git diff --check: clean.

Scope and evidence

This change is limited to per-atom energy allocation and regressions. It leaves the system energy, force accumulation, stress accumulation, and neighbor-list construction unchanged. GPU and long-running simulation validation were not performed.

The F002 evidence and native ASE control documents the original invariant failure.

Checklist

  • Docstrings have been added in the Google docstring format.
  • Ruff lint and format checks pass.
  • Regression tests have been added for the fix.

@lil-lon lil-lon left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The fix looks good, but could you please simplify tests as suggested?

Comment on lines +359 to +361
@pytest.mark.parametrize("neighbor_list_fn", [torch_nl_n2, torchsim_nl])
@pytest.mark.parametrize("reduce_to_half_list", [False, True])
def test_isolated_pair_atom_energies(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This path is covered by test_batched_atom_energies_unequal_system_sizes, whose second system is the same isolated pair, so I think you can delete this test.

torch.testing.assert_close(out["energies"].sum(), out["energy"].sum())


@pytest.mark.parametrize("neighbor_list_fn", [torch_nl_n2, torchsim_nl])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The neighbor-list backend parametrization is unnecessary, since the neighbor-list backends are tested separately.
Keeping this test parametrized only over reduce_to_half_list should be enough.

This branch has not been deployed

No deployments
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.

3 participants