Skip to content

fix: Address code review findings (bugs, DRY, lint, docs)#106

Merged
ericchansen merged 2 commits intomasterfrom
fix/code-review-cleanup
Mar 21, 2026
Merged

fix: Address code review findings (bugs, DRY, lint, docs)#106
ericchansen merged 2 commits intomasterfrom
fix/code-review-cleanup

Conversation

@ericchansen
Copy link
Copy Markdown
Owner

Summary

Address findings from a comprehensive multi-model code review (Claude Opus, Gemini 3 Pro, GPT-5.3 Codex). Fixes bugs, eliminates code duplication, tightens lint config, and corrects documentation.

Changes

Bug Fixes

  • License mismatch: pyproject.toml and README.md now correctly say MIT, matching the LICENSE file and upstream nsf-c-cas/q2mm
  • Gaussian parser bare except:: replaced with except StopIteration: to avoid silently masking parse errors (parsers/gaussian.py)
  • .fchk Charge field over-broad match: line.startswith("Charge") also matched "Charges" array sections, silently corrupting the parsed molecular charge. Fixed to "Charge " (trailing space) (optimizers/objective.py)
  • Incomplete _ATOMIC_SYMBOLS: expanded from 22 to 103 elements (Z=1-103). Unsupported atomic numbers now raise ValueError instead of silently producing "X{z}" fallback symbols that cause KeyError downstream (optimizers/objective.py)

DRY Refactors

  • Centralize parameter matching: added match_bond(), match_angle(), match_vdw() to ForceField, eliminating duplicated cascade logic in OpenMM and JAX backends (models/forcefield.py, backends/mm/openmm.py, backends/mm/jax_engine.py)
  • Centralize covalent radii: TinkerEngine._detect_bonds() now imports COVALENT_RADII from molecule.py instead of maintaining a separate dict with inconsistent values (Cl: 0.99 vs 1.02, Br: 1.14 vs 1.20) (backends/mm/tinker.py)

Hygiene

  • Removed empty subpackages: q2mm/forcefields/ and q2mm/io/ contained only __pycache__ but were shipped in the distribution
  • Tightened ruff config: removed global F821 (undefined name) and E722 (bare except) suppressions; moved to per-file ignores for legacy parser modules only (pyproject.toml)
  • Fixed TYPE_CHECKING import: resolved F821 for Q2MMMolecule type hint in forcefield.py

Documentation

  • Updated package structure in docs/getting-started.md (removed phantom forcefields/ and io/ modules)
  • Fixed contradictory performance claim in docs/performance.md (was "1.6x faster", should be "~150x higher throughput")
  • Fixed import path in examples/ethane/README.md (q2mm.io -> q2mm.parsers)

Testing

  • All 356 tests pass, 44 skipped (backend-specific), 0 failures
  • ruff check passes with zero violations

- Fix license mismatch: pyproject.toml and README now correctly say MIT,
  matching the LICENSE file and upstream nsf-c-cas/q2mm
- Fix Gaussian parser bare except: replace with except StopIteration to
  avoid silently masking parse errors
- Fix .fchk Charge field matching: use 'Charge ' (trailing space) to
  avoid matching 'Charges' array sections
- Complete _ATOMIC_SYMBOLS: expand from 22 to 103 elements (Z=1-103),
  raise ValueError on unsupported atomic numbers instead of silent fallback
- Centralize parameter matching: add match_bond/match_angle/match_vdw to
  ForceField, eliminating duplicated logic in OpenMM and JAX backends
- Centralize covalent radii: Tinker backend now imports from molecule.py
  instead of maintaining a separate dict with inconsistent values
- Remove empty q2mm/forcefields/ and q2mm/io/ subpackages
- Tighten ruff config: remove global F821/E722 suppression, use per-file
  ignores for legacy parser modules only
- Fix docs: update package structure, correct performance claim, fix
  import path in ethane example

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 21, 2026 05:15
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

This PR addresses prior review findings by fixing several parsing/metadata bugs, centralizing repeated parameter-matching logic across MM backends, tightening lint configuration, and correcting documentation/examples to reflect the current package structure.

Changes:

  • Fixes parsing correctness issues in Gaussian .out/.fchk handling and improves .fchk element/charge robustness.
  • Refactors OpenMM and JAX backends to reuse centralized ForceField parameter matching helpers; aligns Tinker bond detection radii with the shared molecule model.
  • Updates license metadata and refreshes docs/examples to match the current module layout; tightens Ruff ignores to targeted per-file exceptions.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
README.md Updates stated project license to MIT to match LICENSE.
q2mm/parsers/gaussian.py Replaces bare except with except StopIteration during parsing.
q2mm/optimizers/objective.py Fixes .fchk charge header matching; expands atomic number → symbol mapping and raises on unsupported Z.
q2mm/models/forcefield.py Adds centralized match_bond/match_angle/match_vdw helpers; fixes TYPE_CHECKING import for molecule typing.
q2mm/backends/mm/tinker.py Uses shared COVALENT_RADII from molecule.py for bond detection.
q2mm/backends/mm/openmm.py Delegates bond/angle/vdW matching to ForceField.match_* helpers (DRY).
q2mm/backends/mm/jax_engine.py Delegates bond/angle/vdW matching to ForceField.match_* helpers (DRY).
pyproject.toml Updates license metadata/classifier; tightens Ruff ignores to per-file F821 for legacy parsers.
examples/ethane/README.md Fixes import path (q2mm.parsers instead of removed q2mm.io).
docs/performance.md Corrects OpenMM vs Tinker performance statement.
docs/getting-started.md Updates package tree to reflect current modules (removes non-existent packages).

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

Comment thread q2mm/models/forcefield.py Outdated
- Remove redundant element fallback in match_vdw() — get_vdw() already
  handles atom_type→element fallback internally (review feedback)
- Add per-file F821 ignore for test/test_linear_algebra.py (legacy test
  with undefined refs, caught by tightened global lint config)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ericchansen ericchansen merged commit 96180ee into master Mar 21, 2026
10 checks passed
@ericchansen ericchansen deleted the fix/code-review-cleanup branch March 21, 2026 05:33
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