Skip to content

Fix twelve findings from the 2026-08-17 review - #22

Merged
farrisric merged 1 commit into
mainfrom
fix/review-followup-2026-08-17
Aug 17, 2026
Merged

farrisric merged 1 commit into
mainfrom
fix/review-followup-2026-08-17

Conversation

@farrisric

Copy link
Copy Markdown
Owner

Fixes the twelve high- and medium-severity findings from the 2026-08-17 review of the v1.4.0 changes (the review ran against PR #21, whose content was already rebased onto main).

Replica exchange

  • De Broglie cross-term in the swap criterion. The grand-canonical stationary weight carries Lambda_s^(-3 N_s) per exchangeable species, and Lambda depends on T, so swaps between rungs at different temperatures with different particle counts were biased (e.g. 300/400 K with dN = 10: acceptance off by ~e^4.3). Both BatchedReplicaExchange._accept_swap and the MPI ReplicaExchange._exchange_prob_T now add 3 * sum_s dN_s * ln(Lambda_i,s / Lambda_j,s); lambda_dbs rides on get_state(). Mu ladders were already exact (the term cancels at shared T) and are pinned unchanged; LJ units (Lambda = 1) are unaffected automatically.
  • Non-finite swap exponents warn and reject. The old min(1, exp(nan)) form silently accepted every swap involving a NaN energy; the rewritten comparison would have silently rejected. Neither is acceptable for a diverged relaxation, so a NaN/inf delta now logs a warning naming the pair and rejects.
  • Per-rung dead-ladder detection. The whole-run degeneracy check could not see a ladder dead at individual rungs (early free swaps inflate the cumulative tally forever). The run now snapshots the tallies at its midpoint and warns per rung on zero second-half acceptance, the test the ladder-spacing doc prescribes.
  • Degeneracy warning counts swap attempts, not doubled per-slot tallies (40/40, not 80/80), and is emitted as a single log record so filtering handlers cannot strand the reason line.
  • Replica isolation is checked at construction. A factory written for the MPI class (module-level shared atoms/moves, safe under one-process-per-rank) silently corrupts every replica under batching; sharing atoms, move_selector or cells across replicas now raises.

State and cells

  • Lossless restart round-trip. set_state restores step and the exchange tallies when the dict carries them; ReplicaExchange strips those slot-bound keys before applying a swap (previously set_state dropped them unconditionally, silently renumbering restarted runs from step 0).
  • is_point_exchangeable defaults on BaseCell, and the molecule moves fall back to is_point_inside for plain duck-typed cells, restoring the pre-1.4.0 single-predicate contract that 1.4.0 broke with an AttributeError.

Alchemi calculators

  • head= with a pre-loaded MACEWrapper raises. It was silently ignored, so a fine-tuned multihead model shared as a wrapper evaluated on head 0, the pretrain head.
  • The energy_only guard message no longer suggests an impossible workaround ("set energy_only on all of the sharers" trips the same guard).
  • Forces-stripped models are refused where forces are needed: AlchemiFCalculator.__init__ rejects a wrapper whose active_outputs lacks forces (the .model of an energy-only calculator), and run_md on an energy-only calculator raises up front instead of dying steps deep inside nvalchemi.

Docs

  • The ladder-spacing reference implementation now includes the mu_max endpoint (the np.arange fencepost dropped the top rung by up to a full spacing), sorts descending isotherms (desorption scans), and clips noise-negative dN/dmu instead of propagating NaN into a cryptic crash. Verified by executing the snippet: endpoint exact, noisy input stays finite, descending input matches ascending.

Tests

11 new tests: analytic swap probabilities with and without the cross-term, slot-order symmetry, NaN rejection with warning, per-rung second-half detection, un-doubled warning counts, replica-isolation refusal and acceptance, restart round-trip for both ensembles, and the duck-typed-cell fallback. Full suite: 198 passed; flake8 clean.

Replica exchange:
- Add the de Broglie cross-term 3*sum_s(dN_s)*ln(Lambda_i/Lambda_j) to the
  grand-canonical swap criterion in both BatchedReplicaExchange and the MPI
  ReplicaExchange (lambda_dbs now rides on get_state). Mu ladders were
  already exact (the term cancels at shared T); temperature and joint
  ladders with fluctuating N were biased.
- Reject non-finite swap exponents with a warning instead of silently
  inverting the NaN behavior (the old min(1, exp(nan)) accepted every swap).
- Warn per rung on zero second-half swap acceptance (midpoint tally
  snapshot): the whole-run check could not see a ladder dead at one rung.
- Report swap attempts, not doubled per-slot tallies, in the degeneracy
  warning, and emit it as a single log record.
- Refuse factories that share atoms, move_selector or cells across replicas
  under batching.

State and cells:
- set_state restores step and exchange tallies when present (lossless
  restart round-trip); ReplicaExchange strips them on swaps instead.
- is_point_exchangeable defaults on BaseCell, and the molecule moves fall
  back to is_point_inside for duck-typed cells (pre-1.4.0 contract).

Alchemi calculators:
- Raise when head= is passed with a pre-loaded MACEWrapper (it was silently
  ignored, evaluating multihead models on the pretrain head).
- Drop the impossible workaround from the energy_only guard message.
- Refuse forces-stripped models in AlchemiFCalculator and in run_md.

Docs:
- Ladder-spacing reference: include the mu_max endpoint (fencepost), sort
  descending isotherms, clip noise-negative dN/dmu.
@farrisric
farrisric merged commit 876558f into main Aug 17, 2026
4 checks passed
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.

1 participant