Skip to content

fix(hessian): Address code review findings from PRs #81-83#84

Merged
ericchansen merged 2 commits intomasterfrom
fix/review-findings
Mar 20, 2026
Merged

fix(hessian): Address code review findings from PRs #81-83#84
ericchansen merged 2 commits intomasterfrom
fix/review-findings

Conversation

@ericchansen
Copy link
Copy Markdown
Owner

Summary

Fixes all 8 code review findings from GPT-5.4 and Gemini 3 Pro reviews of PRs #81-83.

Changes

Bug fixes

  • Silent method fallback (GPT HIGH): invert_ts_curvature(method="E") or any typo silently fell through to Method C. Now raises ValueError for unsupported values.
  • Bogus element-wise check (Gemini HIGH): if not (inv_curv_hessian >= 0.0).all() checked matrix elements, not eigenvalues. Off-diagonal coupling force constants are routinely negative, so this fired false warnings for every real molecule. Removed entirely.
  • Fragile index-based locking (both reviewers): detect_problematic_params returned list indices; lock_params used those indices to copy values between force fields with potentially different orderings. Now uses canonical param keys (param.key tuples).

Docstring/accuracy fixes

  • Units mismatch (Gemini LOW): replace_neg_eigenvalue docstring said "Hartree/bohr^2/amu^-1" but HESSIAN_CONVERSION has no mass terms. Clarified: operates on Cartesian Hessian eigenvalues, not mass-weighted.
  • lock_params honesty: Docstring previously claimed params were "frozen" -- they're only copied. Clarified that set_param_vector can still overwrite them; true freezing requires excluding params from the optimization vector.

Test improvements

  • Literature validation (GPT MEDIUM): Added TestLiteratureValidation class with independently sourced force constant ranges (MM3/IR spectroscopy) so ethane tests aren't purely self-referential.
  • Natural sort (Gemini MEDIUM): Jaguar .in file glob in parity tests now uses natural sort key so 2.in < 10.in regardless of zero-padding.
  • Remove from __future__ import annotations (user): Removed from test_method_d_e.py and test_ethane_seminario.py -- not needed on Python 3.10+. Kept in hessian.py where it's required for TYPE_CHECKING pattern.

Testing

  • All 256 tests pass (251 baseline + 5 new literature validation tests)
  • 20 skipped (Schrodinger-gated)
  • Ruff lint clean

- Validate method param in invert_ts_curvature; raise ValueError for
  unsupported values instead of silently falling through to Method C
- Remove bogus element-wise >= 0 check on Hessian matrix (off-diagonal
  coupling constants are routinely negative; this fired false warnings)
- Change detect_problematic_params/lock_params to use canonical param
  keys instead of fragile list indices
- Clarify lock_params docstring: it copies values but does not freeze
  params during optimization (honest about current limitation)
- Fix docstring units: Cartesian Hessian eigenvalues, not mass-weighted
- Use natural sort for Jaguar .in file glob in parity tests
- Remove unnecessary from __future__ import annotations in test files
- Add literature-validated force constant range tests for ethane

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Addresses prior review findings around TS Hessian inversion behavior, parameter locking robustness, and test reliability/validation.

Changes:

  • Make invert_ts_curvature() reject unsupported methods (no silent fallback) and remove a misleading element-wise negativity check.
  • Switch “problematic parameter” detection/locking from list indices to canonical parameter keys.
  • Improve tests: add natural sorting for Jaguar inputs and add independent literature-based validation for ethane.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
test/test_method_d_e.py Updates tests to use key-based locking/problem detection; adds invalid-method assertion.
test/integration/test_seminario_parity.py Adds a natural sort key for .in files to stabilize parity/benchmark ordering.
test/integration/test_ethane_seminario.py Adds independent literature-range validation tests for ethane force constants.
q2mm/models/hessian.py Enforces method validation, clarifies docs/units, and migrates problematic detection/locking to key-based mapping.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread q2mm/models/hessian.py
Comment thread q2mm/models/hessian.py
Comment thread test/test_method_d_e.py Outdated
Comment thread test/integration/test_ethane_seminario.py Outdated
Comment thread test/integration/test_ethane_seminario.py Outdated
Comment thread test/integration/test_ethane_seminario.py Outdated
Comment thread test/integration/test_ethane_seminario.py Outdated
- Warn when lock_params source FF is missing requested keys
- Align docstring reference to co.HESSIAN_CONVERSION (matches import alias)
- Strengthen test_all_problematic with distinct element types for full
  coverage of multi-problem detection
- Use next(..., None) + explicit assert in literature validation tests
  for clearer failure messages

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ericchansen ericchansen merged commit c68904f into master Mar 20, 2026
5 checks passed
@ericchansen ericchansen deleted the fix/review-findings branch March 20, 2026 16:43
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.

2 participants