Refactor/consolidate neq engine#114
Merged
Merged
Conversation
…ers generated by the python part -- QligFEP
Non-equilibrium method is now folded into the md.f90 as implemented in the previous commit: bd0c492
…n, similar to the equilibrium analyzer
…s both analyzers (neq and equilibrium). Also, remove duplicated method from analyze_FEP caused by merge conflicts during rebase
…p-by core to avoid cases where `ranks` > `available cores` caused by OpenMPI in some instances of resource allocation from Slurm
…nning templates to correctly halt the run if it crashes
…uster default)) We default to 5 NEQ replicates, each producing 2 switches (forward and reverse) = 10 independent switches, and each switch runs on a single MPI rank. With an 8-core allocation those 10 switches ran as 8 + 2: a full wave followed by a starved tail wave of 2 (6 cores idle), which doubled the switch-phase wall time and made NEQ slower than the cost-matched equilibrium run. Size the allocation to hold every switch in one concurrent wave instead: NTASKS = max(2*neq_reps, cluster default). Growing to the switch count removes the tail wave. Flooring at the cluster default keeps at least the billed core count -- on clusters that always bill a fixed minimum (e.g. SNELLIUS bills 16), the equilibration (parallel qdynp) uses all billed cores while the switch phase runs 2*neq_reps of them in a single wave. neq_reps is unchanged, so the cost-matched protocol from the manuscript is preserved; any billed cores beyond 2*neq_reps are simply idle during switching. Also: the packing warning now fires only on a genuine partial tail wave (i.e. a hand-edited --ntasks-per-node below the switch count), and the tutorial documents the core mapping and per-cluster --ntasks-per-node tuning.
…oach, same as in Q defaults
There was a problem hiding this comment.
Pull request overview
This PR consolidates the NEQ² (fast-switching) implementation into the main qdyn engine (runtime-selected via an optional [lambda_scaling] input section), removing the standalone qdyn_neq build target. It also hardens the SLURM run scripts and unifies equilibrium/NEQ run-status diagnostics and reporting across the Python analyzers.
Changes:
- Refactors the Fortran engine to support NEQ fast-switching mode inside
qdyn, removes theqdyn_neqtarget from thesrc/q6makefile, and updates documentation to match. - Updates QligFEP NEQ job generation and SLURM templates to use
qdynpfor equilibration and serialqdynfor switching, adds robust footers/traps, and standardizes diagnostics parsing viaIO.read_slurm_diagnostics. - Extends NEQ analysis to optionally write
<mapping>_ddG.jsonoutputs (like equilibrium analysis) and adds/updates tests and tutorials.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/Tyk2/README.md | Updates NEQ tutorial text to reference serial qdyn fast-switching mode and revises work-units note. |
| tutorials/Tyk2/neq2/README.md | Adds/updates NEQ² tutorial content and explains the consolidated engine + recommended defaults. |
| test/qligfep/test_softcore_methods.py | Refactors CLI invocation construction and updates softcore-method expectations. |
| test/qligfep/test_io.py | Adds coverage for the shared SLURM footer parser (read_slurm_diagnostics). |
| test/neq/test_neq_inputs.py | Updates NEQ run-script assertions for qdynp/qdyn and core mapping behavior; adds new allocation sizing tests. |
| test/neq/test_neq_analysis.py | Adds tests for injecting NEQ results into <mapping>_ddG.json and NaN→null serialization. |
| src/QligFEP/settings/settings.py | Rewires engine path placeholders for NEQ (adds QDYNP, points QDYN_NEQ at serial qdyn). |
| src/QligFEP/qligfep.py | Updates NEQ runfile generation, adds allocation sizing logic, and improves mpirun mapping/binding strings. |
| src/QligFEP/IO.py | Introduces shared RUN_FAILURE_KEYWORDS and read_slurm_diagnostics used by both analyzers. |
| src/QligFEP/INPUTS/run.sh | Hardens run script with set -eo pipefail, EXIT-trap footer, and stricter completion checks. |
| src/QligFEP/INPUTS/run_neq.sh | Updates to qdynp/qdyn, adds EXIT-trap footer, hardens execution, and improves switch batching/diagnostics. |
| src/QligFEP/CLI/parser_base.py | Updates CLI help text to reference consolidated serial qdyn NEQ mode. |
| src/QligFEP/analyze_neq.py | Switches to shared diagnostics parser and adds optional mapping JSON injection output. |
| src/QligFEP/analyze_FEP.py | Replaces duplicated SLURM parsing logic with read_slurm_diagnostics and uses shared plotting helper. |
| src/q6/md.f90 | Adds NEQ lambda scheduling + work accumulation into qdyn, adds guardrails, and updates abnormal termination behavior. |
| src/q6/makefile | Removes qdyn_neq target and associated object rules from build targets and move steps. |
| README.md | Updates top-level NEQ² description to reflect consolidated qdyn fast-switching mode. |
Comments suppressed due to low confidence (2)
src/q6/md.f90:15323
- Same portability issue here:
write(0, ...)uses a non-standard unit number. Prefer writing to standard output (orerror_unit) so the message is reliably emitted across compilers.
112 write(0,'(a)') 'Aborting due to errors reading restart file.'
stop 255
src/q6/md.f90:15110
schedule_lambdacomputesratioin single precision and mixes it withreal(8)lambdas/endpoints. Keepingratioinreal(8)avoids unnecessary rounding in the λ schedule and work integral inputs.
real :: ratio
ratio = real(step) / real(nsteps)
if (scaling_parameter == 'sigmoidal') ratio = sigmoid_rescale(ratio, L_sigmoid)
lam(1) = endpoint1 * (1-ratio) + (1-endpoint1)*ratio
lam(2) = 1.0_8 - lam(1)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+479
to
+482
| ! write to stderr (unit 0) and exit non-zero, mirroring the MPI_Abort above. The "abnormally" | ||
| ! text is what QligFEP.IO.read_slurm_diagnostics keys on to label the run CRASHED. | ||
| write(0,'(a)') 'qdyn terminated abnormally' | ||
| stop 255 |
Comment on lines
+15087
to
+15096
| real function sigmoid_rescale(u, L) | ||
| implicit none | ||
| real, intent(in) :: u ! switch progress in [0,1] | ||
| real(8), intent(in) :: L ! steepness parameter | ||
| real :: s, s0, s1 | ||
| s = 1.0 / (1.0 + exp(-(2.0*u - 1.0)*L)) ! raw sigmoid | ||
| s0 = 1.0 / (1.0 + exp(+L)) ! sigmoid at u=0 | ||
| s1 = 1.0 / (1.0 + exp(-L)) ! sigmoid at u=1 | ||
| sigmoid_rescale = (s - s0) / (s1 - s0) | ||
| end function sigmoid_rescale |
Comment on lines
+534
to
+536
| if args.json_file: | ||
| results_file = args.json_file.replace(".json", "_ddG.json") | ||
| populate_mapping_json(df, args.json_file, results_file) |
| # equilibration uses the parallel engine across all cores; switches use the serial engine | ||
| assert "qdynp=" in script and "$qdynp " in script # qdynp (parallel) for equilibration | ||
| assert "qdyn=" in script and "$qdyn " in script # qdyn (serial) for switches | ||
| # switches are packed one-per-core via mpirun binding. We assrt on the rendered launch line |
Comment on lines
+419
to
+420
| > **Note on work units.** The switching work is in kcal/mol, so the analyzer applies the | ||
| > physically consistent BAR factor `beta = 1/(k_B*T)` by default (`--work-units kcal`). |
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.
Consolidate the NEQ engine into
qdynThis PR removes the previous binary
qdyn_neqin favor of having the NEQ2 protocol done by qdyn.Summary
Non-equilibrium (fast-switching) FEP was previously carried by a standalone
qdyn_neqbinary compiled from a 16k-line
md_neq.f90— a near-total copy ofmd.f90that had to bekept in sync by hand. This PR folds fast-switching into the main
qdynengine, selected atruntime by an optional
[lambda_scaling]section in the.inpfile, and deletesmd_neq.f90/ theqdyn_neqtarget entirely.Net change: +690 / −16,750 lines. NEQ physics and the cost-matched manuscript protocol
are unchanged; the switching engine is now a mode of
qdyn, not a separate build.Alongside the consolidation, this branch hardens the run scripts and unifies run-status
reporting across the equilibrium and NEQ analyzers.
Fortran engine (
src/q6)md.f90: NEQ is now gated byequilibrium_simulation(.true.unless the input has a[lambda_scaling]section). When active,pot_energydrives the two-state λ schedule vianew
schedule_lambda/sigmoid_rescalehelpers, accumulates thedU/dλ · dλworkintegral in the main dynamics loop, and
write_outreports the switch-tracking λ withoutmutating
EQ.numnodes > 1), withnstates ≠ 2,or with minimization enabled — the work integral only sums correctly on a single serial
rank with exactly two states.
qdynnow exits non-zero on abnormal termination (dieand restart-readfailures write to stderr and
stop 255), so a crashed serial run can no longer be recordedas successful. The
"abnormally"marker is what the Python diagnostics key on.makefile:qdyn_neq,md_neq.o, and theqdyn_neq.orule removed from all targets(
all,debug,test,move2).Python (
src/QligFEP)settings.py,qligfep.py): NEQ run scripts now reference two enginesby name —
$qdynp(parallel) for fixed-λ equilibration and$qdyn(serial) for theswitches.
QDYN_NEQpoints at the consolidated serialqdyn.2 × neq_repsindependent switches;the allocation is now sized to
max(2 × neq_reps, cluster default)so all switches run inone concurrent wave instead of a starved tail wave that doubled switch-phase wall time.
neq_reps(and thus the manuscript's cost-matched protocol) is unchanged.--map-by core --bind-to coreto avoid launch failures whenSLURM splits allocated cores unevenly across sockets (
ranks > cores-on-socket).run.sh,run_neq.sh):set -eo pipefail; a footer emitted froman
EXITtrap (written even on mid-job abort); completion confirmed by"terminated normally"in the log rather than exit code; equilibration failures now halt the chain (noswitching from a missing
eq6checkpoint), while a partial switch loss is tolerated assampling noise and logged.
IO.read_slurm_diagnostics+RUN_FAILURE_KEYWORDS(nowalso catching MPI-launch failures) is used by both
analyze_neqandanalyze_FEP, soequilibrium and NEQ runs are classified identically. Removed the duplicated status logic
from each analyzer.
analyze_neqnow writes per-edgeQ_ddG_avg/sem/stdinto a<name>_ddG.json,matching the equilibrium analyzer, so a mapping JSON alone (no experimental key) produces
machine-readable results.
Tests
test/qligfep/test_io.py(+97): coverage forread_slurm_diagnosticsfooter parsing andfailure-marker precedence.
test/neq/test_neq_analysis.py,test/neq/test_neq_inputs.py: shared-diagnostics andupdated input-generation assertions.
test/qligfep/test_softcore_methods.py: adjusted for the consolidated engine.Docs
tutorials/Tyk2/neq2/README.md) documenting the production defaults,the cost-matching rationale, and the switch→core mapping.
qdyn_neqbinary updated to describeqdyninfast-switching mode.
Reviewer notes / open item
Work-integral window is preserved verbatim. The work integral accumulates only over
istep ∈ [2, nsteps-2], dropping the first two steps and the final step. This was keptexactly as in the original NEQ engine; whether the truncation is intentional is still
unresolved and flagged in a code comment in
md.f90. Not changed in this PR — notedhere so a reviewer can weigh in.