chore: remove dead code, add geometry/fchk tests, CI coverage#285
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes provably-dead geometry non-convergence penalty plumbing in JaxLoss, adds unit tests for core geometry and Gaussian .fchk parsing, and wires pytest coverage reporting into the Tier 1 CI core test job.
Changes:
- Remove unused geometry non-convergence penalty constant/branching from
q2mm/optimizers/jaxloss.py. - Add new core tests for
q2mm.geometryandq2mm.io.fchk.parse_fchk(including TS Hessian eigenvalue + error paths). - Enable
pytest-covcoverage reporting in CI and uploadcoverage.xmlfrom the Python 3.12 matrix entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
test/test_io_fchk.py |
Adds .fchk parser tests (GS/TS fixtures + error cases). |
test/test_geometry.py |
Adds unit tests for bond length/angle/dihedral helpers. |
q2mm/optimizers/jaxloss.py |
Removes dead non-convergence penalty wiring for geometry relaxation. |
q2mm/diagnostics/systems.py |
Documents _MM3_TYPE_MAP atom-type mapping convention with examples/source. |
pyproject.toml |
Adds pytest-cov to dev dependencies. |
.github/workflows/ci.yml |
Runs core tests with coverage and uploads coverage artifact. |
Comments suppressed due to low confidence (1)
q2mm/optimizers/jaxloss.py:60
_relax_coords()still documents that callers "should add a penalty" when the inner solver does not converge, but JaxLoss no longer applies any non-convergence penalty (and_GEOM_NONCONV_PENALTYwas removed). This docstring is now misleading about the actual behavior.
def _relax_coords(energy_fn, params, coords0): # noqa: ANN001, ANN202
"""Return the relaxed geometry and convergence flag for a molecule.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ericchansen
force-pushed
the
chore/code-quality-round
branch
from
268d808 to
a33edec
Compare
ericchansen
force-pushed
the
chore/code-quality-round
branch
from
a33edec to
47445b1
Compare
ericchansen
force-pushed
the
chore/code-quality-round
branch
from
47445b1 to
c868a29
Compare
- Remove _GEOM_NONCONV_PENALTY constant and its usage in jaxloss.py (was hardcoded to 0.0, multiplied by n_geom_refs — always zero) - Remove dead n_geom_refs computation from _build() - Add _MM3_TYPE_MAP docstring in diagnostics/systems.py - Add test_geometry.py (14 tests): bond_length, bond_angle, dihedral_angle - Add test_io_fchk.py (10 tests): parse_fchk with ethane GS/TS fixtures - Add pytest-cov to dev dependencies - Add --cov flags to CI test-core job with coverage.xml artifact upload - Suppress ANN lint rules for test/ (consistent with existing test style)
ericchansen
added a commit
that referenced
this pull request
May 27, 2026
…from noise Add post-hoc repeated ObjectiveFunction evaluation for convergence regeneration so q2mm#284 §2 noise findings from q2mm#283 can be reported as measurement uncertainty instead of single-call verdicts. The validation JSON keeps the legacy initial_obj_score, final_obj_score, and improvement_pct fields for existing consumers, and adds: - initial_obj_score_mean, initial_obj_score_ci95 - final_obj_score_mean, final_obj_score_ci95 - improvement_pct_mean, improvement_significant Reports the sample mean (not median) paired with a Student-t 95% CI half-width — the t-distribution describes the sampling distribution of the mean, not the median. For n ≤ 10 with the bounded engine noise we measure here, sample mean and median are nearly identical; the mean is the right center to pair with a t-CI. ObjectiveFunction.history is restored between samples by truncating back to its original length (O(1)) rather than copying-then-replacing (O(len)) — important when the optimizer has accumulated many evaluations. Validation: - ruff check + format clean - 680 unit tests pass (24 new tests from #285 included) - ch3f smoke run with --n-evals 3 produces both legacy and new fields; ci95 ≈ 1e-15 (deterministic single-mol system); SIGNIFICANT verdict as expected (99.83 % vs ~0 CI) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ericchansen
added a commit
that referenced
this pull request
May 27, 2026
…from noise Add post-hoc repeated ObjectiveFunction evaluation for convergence regeneration so q2mm#284 §2 noise findings from q2mm#283 can be reported as measurement uncertainty instead of single-call verdicts. The validation JSON keeps the legacy initial_obj_score, final_obj_score, and improvement_pct fields for existing consumers, and adds: - initial_obj_score_mean, initial_obj_score_ci95 - final_obj_score_mean, final_obj_score_ci95 - improvement_pct_mean, improvement_significant Reports the sample mean (not median) paired with a Student-t 95% CI half-width — the t-distribution describes the sampling distribution of the mean, not the median. For n ≤ 10 with the bounded engine noise we measure here, sample mean and median are nearly identical; the mean is the right center to pair with a t-CI. ObjectiveFunction.history is restored between samples by truncating back to its original length (O(1)) rather than copying-then-replacing (O(len)) — important when the optimizer has accumulated many evaluations. Validation: - ruff check + format clean - 680 unit tests pass (24 new tests from #285 included) - ch3f smoke run with --n-evals 3 produces both legacy and new fields; ci95 ≈ 1e-15 (deterministic single-mol system); SIGNIFICANT verdict as expected (99.83 % vs ~0 CI) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Remove dead code, improve documentation, add missing tests, and wire up coverage reporting in CI.
Changes
Dead code removal
_GEOM_NONCONV_PENALTYconstant fromjaxloss.py— was hardcoded to0.0, so the penalty branch (0.0 * n_geom_refs) always contributed zero. Also removes the now-unusedn_geom_refscomputation from_build().Documentation
_MM3_TYPE_MAPindiagnostics/systems.pyexplaining the element + bond-count → MM3 type mapping convention, with examples and literature reference.New tests
test_geometry.py— 14 tests coveringbond_length,bond_angle,dihedral_angle(edge cases: collinear, sign convention, symmetry, numpy arrays).test_io_fchk.py— 10 tests coveringparse_fchkwith ethane GS/TS fixtures (symbols, coords shape, Angstrom units, Hessian symmetry, TS negative eigenvalue, error paths).CI coverage
pytest-covto dev dependencies.--cov=q2mm --cov-report=term-missing --cov-report=xmlto thetest-coreCI job.coverage.xmlas an artifact from the Python 3.12 matrix entry.No behavioral changes
All modifications are additive (tests, docs, CI) or remove provably dead code.