fix: Address multi-model architecture review findings

[ericchansen]

Summary

Address critical and medium findings from a multi-model code review (Claude Opus 4.6, GPT-5.4, Gemini 3 Pro) comparing the refactored codebase against upstream.

Changes

Critical Fixes

Fix	Detail
TSFF bounds	Widened bond_k from [0, 50] to [-50, 50] and angle_k from [0, 10] to [-10, 10] -- negative force constants are essential for transition state force fields
Torsion support	TorsionParam now included in param vector, I/O (MM3 + Tinker), bounds, and get_torsion() lookup. Each Fourier component (V1/V2/V3) is a separate optimizable parameter
Observable types	Added torsion_angle to ObjectiveFunction with proper 360-degree wraparound in residuals and a _dihedral_angle() geometry helper

Bug Fixes

Fix	Detail
correlate_energies	select_group_of_energies now iterates ["e", "eo", "ea", "eao"] instead of just ["e", "eo"] -- absolute energies were never zero-referenced before scoring
Eigenvalue strictness	replace_neg_eigenvalue now raises ValueError on multi-negative eigenvalue Hessians by default (strict=True), with warnings.warn instead of print

Documentation

• Documented scoring divergence between modern ObjectiveFunction (raw residuals) and legacy scoring.py (energy correlation + normalization)
• Documented upstream gradient methods (lstsq, lagrange, svd) and their scipy equivalents in scipy_opt.py docstring + README
• Added optimization methods table to README

Testing

• 153 passed, 8 skipped, 1 xfailed
• 11 new tests added across test_models.py and test_linear_algebra.py
• Ruff lint + format: clean
• Golden fixture regenerated (optimizer score improved 1.77 -> 0.93 due to bounds fix)

Files Changed (11)

• q2mm/models/forcefield.py -- TSFF bounds, torsion in param vector
• q2mm/models/ff_io.py -- Torsion extraction/export for MM3 + Tinker
• q2mm/models/param.py -- Allow negative values for bf/af ptypes
• q2mm/models/hessian.py -- Strict eigenvalue check
• q2mm/linear_algebra.py -- Strict eigenvalue check (duplicate of hessian.py)
• q2mm/optimizers/objective.py -- Torsion observable, scoring docs
• q2mm/optimizers/scoring.py -- correlate_energies fix
• q2mm/optimizers/scipy_opt.py -- Lost methods documentation
• test/test_models.py -- 6 torsion + 3 bounds tests
• test/test_linear_algebra.py -- 2 eigenvalue strictness tests
• README.md -- Optimization methods section

[Copilot]

Pull request overview

This PR addresses review findings in Q2MM’s refactored multi-model architecture by extending TSFF parameter support (negative force constants), adding torsion parameters/observables end-to-end, tightening eigenvalue handling, and documenting optimizer/scoring differences vs upstream.

Changes:

• Extend ForceField parameterization: TSFF-friendly bounds (negative bond/angle k) and torsions included in param vector, bounds, lookup, and MM3/Tinker I/O.
• Add torsion-angle observable support in the scipy objective function with 360° wraparound residual handling.
• Fix/clarify legacy scoring behavior (energy correlation across all energy types) and make negative-eigenvalue handling strict-by-default with warnings instead of prints; update README/scipy optimizer documentation.

Reviewed changes

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

Show a summary per file

File	Description
q2mm/models/forcefield.py	Adds torsions to the model (vector/bounds/lookup) and widens bounds for TSFF usage.
q2mm/models/ff_io.py	Imports/exports torsion components for MM3 .fld and Tinker .prm via legacy df params.
q2mm/models/param.py	Allows negative ranges for bf/af parameter types to support TSFF.
q2mm/models/hessian.py	Adds strict handling for multi-negative eigenvalues and switches prints to warnings.
q2mm/linear_algebra.py	Mirrors strict eigenvalue handling and warning behavior.
q2mm/optimizers/objective.py	Adds torsion-angle observable type and dihedral geometry helper; documents scoring differences.
q2mm/optimizers/scoring.py	Fixes energy grouping to include ea/eao for correlation/zero-referencing.
q2mm/optimizers/scipy_opt.py	Documents mapping from upstream gradient methods to scipy equivalents.
test/test_models.py	Adds tests for negative bounds, torsion vector/bounds/lookup, and MM3 torsion parsing.
test/test_linear_algebra.py	Adds tests for strict vs non-strict behavior on multi-negative eigenvalues.
README.md	Documents supported optimization methods and notes legacy preservation.

Comments suppressed due to low confidence (2)

q2mm/linear_algebra.py:69

• If there are no negative eigenvalues, neg_indices is empty and the else branch will raise IndexError at neg_indices[0][0]. Consider explicitly handling the no-negative case (e.g., return unchanged eigenvalues, or raise a clear ValueError).

        index_to_replace = np.argmin(eigenvalues)
    else:
        index_to_replace = neg_indices[0][0]
    replaced_eigenvalues = copy.deepcopy(eigenvalues)

q2mm/models/hessian.py:143

• If there are no negative eigenvalues, neg_indices is empty and the else branch will raise IndexError at neg_indices[0][0]. Consider explicitly handling the no-negative case (e.g., return unchanged eigenvalues, or raise a clear ValueError).

        index_to_replace = np.argmin(eigenvalues)
    else:
        index_to_replace = neg_indices[0][0]
    replaced_eigenvalues = copy.deepcopy(eigenvalues)

---

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

You can also share your feedback on Copilot code review. Take the survey.