Skip to content

Add integration tests checking the physical observables resulting from different the integrators#521

Open
thomasloux wants to merge 18 commits intoTorchSim:mainfrom
thomasloux:feat/add-physical-validation
Open

Add integration tests checking the physical observables resulting from different the integrators#521
thomasloux wants to merge 18 commits intoTorchSim:mainfrom
thomasloux:feat/add-physical-validation

Conversation

@thomasloux
Copy link
Copy Markdown
Collaborator

@thomasloux thomasloux commented Mar 24, 2026

Summary

I've added tests to evaluate the correctness of integrators. I'm using physical_validation, used in gromacs test suite. The code is WIP, so that people can try it easily and update some parameters. In the future, the systems studied and the parameters will need to be fixed, or used the standard parameters. Note: this is important because the default parameters may be changed in an undesirable way.

What to do next?

  • Check that the chosen parameters are correct default one.
  • Run for longer to have convergence of the integrator.
  • Choose the optimal pressure to correctly evaluate NPT integrators.
  • Change default parameters for some integrators, most importantly for NPT NH.

Notes

  • Parameters for NPT langevin are most likely wrong, as noted in Wrong unit of default b_tau in npt_langevin_init #453.
  • C-Rescale Anisotropic does not seem to work, crystal general tend to grow too large. I haven't checked the trajectories, but this is something I've realized during development. Maybe we should add a warning when a user chooses NPT anisotropic for now.
  • Langevin parameters need to be more studied, the cell volume variation is far from correct.
  • I should add how to estimate the isothermal_compressibility for c-rescale, which is easy and fast using QHA and phonopy.
  • I've already tested changes in Fix NPT Nose-Hoover things #520 and it works great. Actually I didn't even try without these changes, but I'm quite convinced the changes in Fix NPT Nose-Hoover things #520 are correct.

@thomasloux thomasloux marked this pull request as draft March 24, 2026 17:30
@thomasloux
Copy link
Copy Markdown
Collaborator Author

In the previous commit:

  • I added equations in the docs. It's easier to read then checking the papers each time.
  • I fix Anisotropic NPT C-rescale: sign error (yes that's stupid). Now it works
  • Fix some factor issues in NPT Langevin. @abhijeetgangan Can you have a look as I think you first implemented them?
  • I've updated the default parameters, they work well with LJ Argon. I don't know for other MLIP, but that's the best I can do for now.

What's next?

Regarding integrators

  • NPT Langevin does not provide correct pressure sampling (more precisely volume fluctuations). I'm trying to compare the implementation to LAAMPS but could not yet find the issue.

Docs

  • Docs need to be super explicit about units. Any parameters in integrator must be in the time (or inverse time) units so ps * MetalUnits.time. I highly suspect most people make the error of using ts.integrate(timestep=dt) with dt=0.001 and then use some factor*dt for integrator argument, because timestep is automatically converted to torch-sim units (multiplied by MetalUnits.time in integrate).

@thomasloux thomasloux marked this pull request as ready for review April 3, 2026 14:53
@thomasloux
Copy link
Copy Markdown
Collaborator Author

Ok I've managed to make all integrator pass:

Changelog:
NPT Langevin

  • There was an error with cell_positions shape, initialized as a volume [n_system, 1, 1] and turn into [n_system, 3, 3] when added with force_pressure that was [n_system, 3, 3]
  • Change the volume control to strain: otherwise the volume fluctuation was too low. The mean was correct but not fluctuation. Now with strain the correct thermodynamic system is sampled
    Side note: these problems were in the code from the very first implementation.
  • Added a version fully isotropic NPT Langevin Strain, and a version like LAAMPS that can control independently x,y,z. This make up 2 integrators for NPT Langevin, we can keep it like that or modify it to be one with an argument.

Others:

  • Correct unwrap_positions that was wrong because test was wrong when evaluating modifying cells (ie. NPT simulation)

Note:

  • Lennard Jones Model just got modified to be built from pair_potentials. That's great because it actually corrected a bug in Lennard Jones Model that was providing wrong forces: with the neighbor list it would correct the energy with a factor 1/2 because this matrix is symmetric, but this correction was not on forces, nor stress. That explains why it took me so long to make these tests, the model was not even right haha.
  • Lennard Jones is crazy fast on gpu, the performance hardly depends on the number of atoms in the graph (which is the main bottleneck). So tests can run on gpu on relatively large cells. This is not true on cpu, that's why I force the test on GPU.
  • On my h200: 1 min = 10_000 steps of most integrators except Nose Hoover (the substeps take time), so the entire integrator test is actually relatively fast to run, less than an hour with a GPU.

@CompRhys CompRhys changed the title Feat/add-physical-validation Add integration tests checking the physical observables resulting from different the integrators Apr 4, 2026
[project.optional-dependencies]
test = [
"torch-sim-atomistic[io,symmetry,vesin]",
"physical-validation>=1.0.5",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: This package seems unsupported

# Map masses to have batch dimension
M_2 = 2 * state.masses.unsqueeze(-1) # shape: (n_atoms, 1)

# Calculate new cell length scale (cube root of volume for isotropic scaling)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the new beta coming in externalizes this to another method correct?

Comment on lines +627 to +629
# =============================================================================
# NPT Langevin Strain integrator — isotropic logarithmic strain coordinate
# =============================================================================
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: can we remove CC headers and titles? Things like titles if used we need to keep consistent and so easier just not to have them imo

Copy link
Copy Markdown
Member

@CompRhys CompRhys left a comment

Choose a reason for hiding this comment

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

Great work!

@CompRhys CompRhys linked an issue Apr 4, 2026 that may be closed by this pull request
Signed-off-by: Rhys Goodall <rhys.goodall@outlook.com>
@thomasloux
Copy link
Copy Markdown
Collaborator Author

By the way, how do you want to handle the integrator names regarding whether they provide isotropic or anisotropic (length only or length+angle) ? NPT Crescale has these 3, now NPT Langevin supports isotropic and independant length.

@thomasloux
Copy link
Copy Markdown
Collaborator Author

thomasloux commented Apr 5, 2026

By the way, I need to correct the pytest tests. I was lazy running them, but now it only takes 15 minutes on a gpu, so that's fairly short. I'll add a doc with my jupyter notebook to show the plots. Note: there still pass on the notebook I use to make the tests.

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.

Wrong unit of default b_tau in npt_langevin_init Add integrator tests to check correct sampling of thermodynamic ensemble

3 participants