refactor(lr): migrate PME Coulomb to energy+autograd - #123
Merged
Conversation
…0.4.1 Complete the #105 migration for PME: a single energy-only nvalchemiops call leaves positions, charges, and cell in the autograd graph, and every derivative (inference forces/stress, force/stress training losses, dense Hessians, HVPs) flows through the calculator's total-energy autograd. The #106 blocker was an upstream composed-graph charge-gradient bug in nvalchemi-toolkit-ops 0.4.0: on the energy-graph route, PME train-mode forces were wrong by up to 4.477 eV/A (100% of elements) while eval mode, energies, standalone kernels, and Ewald were clean. 0.4.1 reworked the implicated machinery (deterministic _dechain_connected_input_grads replaces the charges.grad_fn heuristic) and the same comparison is clean at the 1e-6 GPU noise floor. - lr.py: PME joins the Ewald energy-graph branch; drop the legacy explicit-terms path, _PeriodicCoulombFunction, _periodic_coulomb_hybrid, and the fixed-charge FD Hessian/HVP helpers; ExternalDerivativeTerms loses its unproduced hessian field; runtime guard raises on nvalchemiops < 0.4.1 (pre-releases of older versions included) since the pip pin cannot protect already-installed environments. - calculator.py: training_derivatives flag is DSF-only; PME strain-input plumbing removed; HVP drops the PME directional-FD block (exact autograd, float64 contract kept); eps is deprecated (warns when passed); stale docstrings/comments rewritten. - derivatives.py: external-terms Hessian merge removed; dense PME Hessian is now returned in the force dtype, matching Ewald since #105. - deps: nvalchemi-toolkit-ops floor 0.4.0 -> 0.4.1 (pyproject + uv.lock). - tests: legacy-path tests replaced with energy-graph equivalents parametrized over ewald+pme (module autograd-vs-FD force gate, autograd Hessian vs force-FD gate, energy-in-graph contract spy, version-guard tests); HVP float64 contract now covers pme. - docs: long_range.md and calculator.md derivative-support sections rewritten; stale fixed-charge/NotImplementedError claims removed. Semantic change: PME dense Hessians and HVPs switch from fixed-charge finite difference to relaxed-charge autograd (include the d^2E/(dq.dr) charge-response), the same contract as DSF and Ewald; values shift slightly against the former FD numbers. Retires the last upstream DeprecationWarning from the deprecated direct-output flags. Fixes #106
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.
Fixes #106.
Summary
Completes the #105 migration for PME: a single energy-only nvalchemiops call leaves positions, charges, and cell in the autograd graph, and every derivative (inference forces/stress, force/stress training losses, dense Hessians, HVPs) flows through the calculator's total-energy autograd.
The #106 blocker was an upstream composed-graph charge-gradient bug in nvalchemi-toolkit-ops 0.4.0. The 0.4.1 release reworked exactly the implicated machinery — the
charges.grad_fn-based_has_potentially_geometry_dependent_chargesheuristic (whose instability matches the issue's "charge-gradient edge sometimesNone, sometimes valid" anomaly) was replaced by a deterministic_dechain_connected_input_gradspass that removes each dependent-input path exactly once. Verified with the calculator-level train-vs-eval force comparison from the issue (spiro crystal,tests/data/2000054.cif, single L40S):Mean abs F is 3.75 eV/A on this system, so the 0.4.0 corruption averaged ~21% of the force magnitude — six orders above the ~1e-6 GPU noise floor — while total energies agreed to 1e-6 and Ewald through the identical code path was clean on both versions. With the bug fixed upstream, the issue's file-a-minimal-reproducer-upstream step is moot.
Changes
aimnet/modules/lr.py: PME joins the Ewald energy-graph branch. Removed the legacy explicit-terms path, the_PeriodicCoulombFunctiontraining wrapper,_periodic_coulomb_hybrid, and the fixed-charge FD Hessian/HVP helpers;ExternalDerivativeTermsdrops its now-unproducedhessianfield.RuntimeErrorwith the upgrade command. The pyproject pin only applies at install time; without the guard an already-installed 0.4.0 would corrupt training forces silently.aimnet/calculators/calculator.py: thetraining_derivativesflag is now DSF-only; the PME strain-input plumbing (_external_strain_inputs) is removed; the HVP drops the PME directional-FD block (PME joins the Ewald exact-autograd float64 contract;epsis retained but unused); stale docstrings rewritten.aimnet/calculators/derivatives.py: external-terms Hessian merge removed (no producer remains).pyproject.toml:nvalchemi-toolkit-ops>=0.4.1.long_range.mdandcalculator.mdderivative-support sections rewritten (both also predated the refactor(lr): migrate Ewald Coulomb to energy+autograd #105 Ewald migration and still claimedNotImplementedErrorHessians).Semantic change
PME dense Hessians and HVPs switch from fixed-charge finite difference to relaxed-charge autograd: they now include the
d^2E/(dq.dr)charge-response coupling, matching DSF and Ewald, so vibrational frequencies / IR are comparable across all LR backends. Values shift slightly against the former FD numbers. This also retires the last upstreamDeprecationWarning(deprecated direct-output flags).Note for existing environments
Environments with 0.4.0 already installed must
pip install -U nvalchemi-toolkit-opsbefore selecting PME; the guard raises with that instruction until then.Review round
A final review pass over the finished diff surfaced and led to: the
uv.lockregeneration (it still pinned 0.4.0 against the new>=0.4.1floor), several stale docstrings/comments describing the removed FD machinery, an honest rephrasing of cross-backend Hessian comparability (Ewald and PME are directly comparable; DSF still differs by its shifted-force truncation near the cutoff), disclosure of the dense-PME-Hessian dtype change (float64 FD block -> force dtype, matching Ewald since #105; the HVP keeps its documented float64 return), guard hardening (pre-releases of too-old versions such as0.4.0rc1are now refused; only truly unparsable versions pass), and anepsDeprecationWarningonhessian_vector_product.Testing
All in an nvalchemi-toolkit-ops 0.4.1 environment, serial on one L40S:
create_graph=Truegates run explicitly:test_ewald_pme_hessian_finite_symmetric_sumrule[pme],test_hvp_matches_dense_periodic[pme],test_ewald_pme_hessian_consistent_with_forces[pme],test_hvp_periodic_returns_float64[pme],test_eval_matches_train_forces_and_stress[pme]: pass.