fix(parsers): Fix Jaguar Hessian unit bug + Check 1 validation test#196
Merged
ericchansen merged 4 commits intomasterfrom Apr 1, 2026
Merged
fix(parsers): Fix Jaguar Hessian unit bug + Check 1 validation test#196ericchansen merged 4 commits intomasterfrom
ericchansen merged 4 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a long-standing Jaguar Hessian unit double-conversion that inflated Seminario-derived force constants, and adds “Check 1” infrastructure to validate evaluation of a published force field (Donoghue 2008) using the OpenMM engine.
Changes:
- Remove Jaguar
.inHessian unit conversion soJaguarIn.get_hessian()returns raw Hartree/Bohr² (AU), matching other QM parsers. - Switch
replace_neg_eigenvalue()default unit system to AU and update related unit tests/fixtures. - Add published-FF provenance docs and a new OpenMM-based integration test for “Check 1” validation.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
q2mm/parsers/jaguar.py |
Returns Jaguar Hessians in AU (Hartree/Bohr²) instead of converting, preventing downstream double conversion. |
q2mm/models/hessian.py |
Changes default eigenvalue-replacement units to AU and updates docstrings accordingly. |
test/test_method_d_e.py |
Updates Method C expectation to AU-based replacement behavior. |
test/test_linear_algebra.py |
Updates unit expectations and adds an explicit units=KJMOLA test. |
test/integration/test_published_ff_validation.py |
Adds “Check 1” integration test that evaluates a published FF with OpenMM and pins results via a golden fixture. |
test/fixtures/seminario_parity/rh_enamide_reference.json |
Regenerates rh-enamide parity fixture with corrected (non-inflated) values. |
validation/published_ffs/README.md |
Documents provenance/status for published FF files used in validation. |
test/fixtures/published_ff/.gitkeep |
Ensures the published-FF fixture directory exists in the repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JaguarIn.get_hessian() was multiplying raw Hessian values by HESSIAN_CONVERSION (9375.83), converting from Hartree/Bohr2 to kJ/mol/A2. Downstream code (Seminario, frequency computation) then treated the result as atomic units, inflating force constants by ~9376x and producing unphysical values (e.g. 13700 mdyn/A for Rh-P instead of ~1.5 mdyn/A). Root cause: Jaguar .in files store Hessians in Hartree/Bohr2 (atomic units), confirmed empirically by computing frequencies from raw values and matching Jaguar .out output exactly. The old misleading docstring said 'kJ/(mol*Angstrom^2)' but that described the post-conversion value, not the raw file format. Changes: - Remove HESSIAN_CONVERSION multiplication from JaguarIn.get_hessian() - Return raw AU to match Gaussian .fchk and Psi4 conventions - Change replace_neg_eigenvalue default units from KJMOLA to GAUSSIAN (AU) since canonical internal Hessian unit is now AU throughout - Regenerate rh_enamide_reference.json with corrected values - Update tests for new default eigenvalue replacement behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add integration test that loads the Donoghue 2008 published Rh-enamide FF directly from mm3.fld and evaluates it against QM reference frequencies using OpenMM. Compares against a Seminario baseline to verify the published (optimized) FF produces a meaningfully lower objective score. Also add validation/published_ffs/README.md documenting provenance of all published FFs in the old Q2MM repo and their readiness for validation (only Rh-enamide has complete training data). The golden fixture is generated on first OpenMM run and pinned for reproducibility checks on subsequent runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…trings - Unpack synthetic_ts_hessian as (H, _, _) to avoid unused variable - Skip test when golden fixture is missing instead of auto-generating - Fix docstring references to match import aliases (co. vs constants.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ericchansen
force-pushed
the
feat/published-ff-validation
branch
from
2bd7a5d to
f7b5d3d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Apr 1, 2026
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.
Summary
Fix a critical Jaguar Hessian unit bug that inflated force constants by ~9376x, and add the Check 1 published force-field evaluation harness for the Rh-enamide training set.
What this PR proves now
JaguarIn.get_hessian()returns raw Hartree/Bohr^2 values, matching the actual Jaguar.infile convention and the Gaussian/Psi4 parser contract.replace_neg_eigenvalue()now defaults to AU, matching the Method C literature convention.Current Check 1 status
The OpenMM evaluation harness is working, but the published MacroModel/MM3* force field does not yet reproduce a better fit than the Seminario baseline under OpenMM. To keep that gap visible without blocking the units fix, the parity assertions are tracked as strict
xfailpromotion gates:Those gates are documented in the follow-up issue linked below.
Golden fixture behavior
The golden fixture is now opt-in rather than auto-generated in normal test runs.
Generate/update it locally with OpenMM via:
That writes
test\fixtures\published_ff\rh_enamide_donoghue2008.json, which should be reviewed and committed separately.Changes
Commit 1: units bug fix
KJMOLAtoGAUSSIANCommit 2: Check 1 harness
test/integration/test_published_ff_validation.pyCommit 3: review/documentation cleanup
xfail)Follow-up
xfailgatesTesting
ruff check q2mm/ test/ scripts/ruff format --check q2mm test scripts examplespython -m pytest test/ -x -q -m "not (openmm or tinker or jax or jax_md or psi4)"