Conversation
| Q = ( | ||
| kT_batched.unsqueeze(-1) | ||
| * torch.square(tau_batched).unsqueeze(-1) ** 2 | ||
| * torch.square(tau_batched).unsqueeze(-1) |
There was a problem hiding this comment.
Good catch, that's actually my fault arghhh
| # F = alpha * (2 * KE) - dU/dV - P*V*d | ||
| return ( | ||
| (alpha * KE_per_system) | ||
| (alpha * 2 * KE_per_system) |
There was a problem hiding this comment.
I'm almost sure this is true
torch_sim/integrators/npt.py
Outdated
|
|
||
| # Compute total DOF for thermostat initialization and a zero KE placeholder | ||
| dof_per_system = torch.bincount(state.system_idx, minlength=n_systems) * dim | ||
| dof_per_system = torch.bincount(state.system_idx, minlength=n_systems) * dim - dim |
There was a problem hiding this comment.
I think that it is something to take into account. Technically then we should consider all dofs reductions from constraints (although constraints are not really supported for NPT). Can you implement something like NPTCRescaleState.get_number_of_degrees_of_freedom ? It considers constraints and automatically applies reduction from Fix center of mass
There was a problem hiding this comment.
I found that NPTNoseHooverState already has get_number_of_degrees_of_freedom methods. is it possible to use this?
There was a problem hiding this comment.
Absolutely. The idea is that you should override this method to call super().get_number_of_degrees_of_freedom - 3
There was a problem hiding this comment.
Fixed it. npt_nose_hoover_init gets SimState which has DOF=3N so i subtracted 3 but npt_nose_hoover_invariant gets NPTNoseHooverState which has DOF=3N-3 so i used as-is.
I tested npt_nose_hoover_invariant and observed a relative total Hamiltonian fluctuation of ~1e-4, which seems quite good. (Though I’m not sure what the practical threshold should be.)
torch_sim/integrators/nvt.py
Outdated
| n_atoms_per_system * state.positions.shape[-1] | ||
| ) # n_atoms * n_dimensions | ||
| dim = state.positions.shape[-1] | ||
| dof_per_system = n_atoms_per_system * dim - dim # 3N - 3 |
|
I think to merge this the values in the npt tests need to be updated. They're set to confirm the algorithm is unchanged and reproducibles but the algo changed here. |
|
let me have a final look on the removal of a |
thomasloux
left a comment
There was a problem hiding this comment.
Good for me after testing that the extra model evaluation was not necessary
|
Thanks alphalm4! |

Summary
md.py: NHC chain mass (Q) initialization:tau^4 -> tau^2(although i believe this barely effects t>0 dynamics)npt.py: cell forceKE -> 2KE.The MTK cell force equation (Martyna–Tobias–Klein, 1994, eq.(2.2) + eq.(2.9)) gives (ignoring V-dependent phi and thermostat part)
The missing factor of 2 halved the kinetic pressure driving cell volume changes, leading to incorrect pressure coupling. Verified against jax-md box_force which uses KE2 = sum(p²/m) directly.
npt.py: Remove redundant model eval in NHC inner stepNHC chain half-steps only rescale momenta -> we do not need to re-eval the model(state). Just calling state.stress is ok.
npt.pyandnvt.pyI feel the DOF should be reduced from
NdtoNd - dsince the COM movement is removed, but I'm not sure (it might be reduced at some other points)I didn't added any test scripts, but I got some internal test results that this version of NPT-NH gives nearly the same density convergence with LAMMPS NPT-NH. It'll be great to be double checked the math things by code maintainers.
Checklist
Before a pull request can be merged, the following items must be checked: