[QL-Balance] Evolve and restart periodic shielding amplitudes - #304
[QL-Balance] Evolve and restart periodic shielding amplitudes#304marjohma wants to merge 5 commits into
Conversation
krystophny
left a comment
There was a problem hiding this comment.
Review verdict: Request changes
Summary:
This PR adds an accepted/trial periodic-amplitude state (periodic_amplitude_state_m) that QL-Balance's time-evolution driver checkpoints into each KinProfiles/<step>/ HDF5 group and restores on restart, plus a periodic_constant_psi_pending flag that forces a unit (constant-ψ) drive on the first TimeEvolution solve. The new module is clean, well-tested (rollback/commit), and the HDF5 write/read paths are internally consistent (group naming matches, reader allocates by ub like gengrid, backward-compat fallback to fort.1000). The core problem is that the evolved amplitude is never actually fed back into the solve, so the documented "evolve and restart" behavior is not realized.
Findings:
-
[major]
QL-Balance/src/base/kim_wave_code_adapter.f90:414,636+wave_code_data_64bit.f90:112,716— The restored amplitude is never applied.periodic_amplitudes%acceptedis written to HDF5 and read back, but nothing ever reads it intokim_run_for_all_modes:drive_scaleis always recomputed fresh byperiodic_drive_scale(which has no memory), and the only alternative is the unit drive. Worse, on restartkim_initialize(line 127) resetsperiodic_constant_psi_pending = .true.afterread_background_profiles_h5_timeevolrestores the saved amplitude, so the first post-restart solve forcesdrive_scale = (1,0);sync_periodic_amplitude_trialthen overwrites the restored trial with 1 andaccept()commits 1. The saved non-unit amplitude survives exactly zero solves. As a result the "restart" feature is functionally a no-op (restart reproduces a fresh unit-drive run) and the doc's claim that "the restored accepted state is used as the trial baseline" is false. Required fix: either seeddrive_scale/kim_periodic_scale_modesfrom the restored accepted amplitude (and clearperiodic_constant_psi_pending) when state is restored, or explicitly document that the amplitude is diagnostic-only and drop the restart claim. -
[minor]
QL-Balance/src/base/time_evolution.f90:239,256— After a rejected/redone step the committed amplitude is stale.sync_periodic_amplitude_trial()(line 197) runs only before the redo loop; inside the loop a failure callsreject()(restoringtrialto the old accepted value) but noget_dql/sync recomputes the trial for the redo. When the loop later exits,accept()at line 256 commits that restored old value, discarding the freshly computed trial (T0) for the actual profiles that were advanced. The accepted record then lags the checkpointed profiles, contradicting the doc's "A successful update commits the scale." Since the amplitude isn't used in physics (finding 1) this is bookkeeping-only, but it should either re-sync after a successful redo or keep T0. -
[minor]
QL-Balance/src/base/kim_wave_code_adapter.f90:415-418— On the first (pending) solve,scale_status = 0is hardcoded, bypassing theperiodic_drive_scaleguard. A degenerate unit current (belowcurrent_floor, which would return status 2) is silently accepted on the constant-ψ step. Harmless for valid inputs but weakens the normalization diagnostics on step one. -
[minor]
docs/benchmarks/2026-08-12-kim-294-periodic-amplitude-restart.mdvstime_evolution.f90:414-448— The doc states "an error-controlled redo … recomputes the unit response" and "the restored accepted state is used as the trial baseline", but the redo loop contains noget_dqland the restored state is never consulted (finding 1). The implementation description is inaccurate; either align the code or the documentation.
Verdict: Request changes — the core feature (restarting/evolving the periodic shielding amplitude) does not actually influence the solved response; the restored amplitude is discarded on the first post-restart solve and never applied, so the documented behavior isn't implemented.
…e-feedback' into HEAD # Conflicts: # QL-Balance/src/base/kim_wave_code_adapter.f90
…e-feedback' into feature/kim-294-periodic-amplitude-restart
krystophny
left a comment
There was a problem hiding this comment.
Review verdict: Approve
Summary
PR #304 ("periodic amplitude restart", KIM #294) adds an accepted/trial complex-amplitude state machine (periodic_amplitude_state_m.f90) for periodic KIM responses in QL-Balance time evolution. It wires the state into the KIM adapter (restore vs. constant-ψ first-step drive), checkpoints accepted/trial amplitudes and normalization diagnostics into the KinProfiles/<1000+step>/ HDF5 group, restores them on restart, rolls back the trial on rejected (redo) steps, and fixes the time-evolution reader to look at the current KinProfiles group (with a fort.1000 fallback). It adds a unit test for the state machine and extends test_kim_adapter to verify restored amplitudes seed the first response. The logic is internally consistent, documented in docs/benchmarks/2026-08-12-kim-294-periodic-amplitude-restart.md, and both tests are registered in CMake.
Findings
-
[minor]
wave_code_data_64bit.f90:686-720(reader) vstime_evolution.f90:650-655(writer) —periodic_normalization_versionandperiodic_phase_policyare checkpointed (with exposed constants intended as guard metadata), but the reader never reads or validates them; it restores amplitudes purely on the presence ofperiodic_amplitude_accepted_real. If the normalization scheme or phase policy changes (version bump), stale amplitudes from an old checkpoint would be silently applied. Recommend checking the stored version/phase policy before restoring. -
[minor]
wave_code_data_64bit.f90:606vs writertime_evolution.f90:585/gengrid.f90:235— The restart reader always buildsf_{m_vals(1)}_{n_vals(1)}/KinProfiles/, but the writer usesh5_mode_groupname, which is"multi_mode"whennumres>1. For multi-mode runs the checkpoint group is never found, so amplitude (and even profile) restart silently degrades to the constant-ψ default. This is inherited from a pre-existing profile-read/write mismatch, but since the PR explicitly stores per-mode amplitudes and its commits stress "multi-mode response state", the restart feature will not actually engage for multi-mode runs. Worth confirming this is acceptable or routing the reader throughh5_mode_groupname. -
[minor]
kim_wave_code_adapter.f90:429-466+time_evolution.f90:284-305— Becausekim_periodic_normalization_relaxationis hardcoded to 1.0 and everydoStepre-runsget_dql(adapter else branch) after the init/restore flags are cleared, the restored/committed amplitude only affects the very first KIM solve (the init-time solve, whose response drives no profile advance). The first real step immediately re-normalizes to the target current and overwrites the state. The accepted/trial accept/reject/checkpoint machinery thus provides essentially no cross-step continuity — its real value is the within-step redo rollback and the checkpointed diagnostics. This matches the documented contract but is worth confirming as intended, since it limits the practical benefit of the restart feature. -
[minor]
kim_wave_code_adapter.f90:438-442— In the constant-ψ branch, whenscale_status == 0,drive_scaleis forced to unit(1,0)butkim_periodic_scale_status(i_mn)is still stored as 0 ("normalized response"), so a deliberately unnormalized first-step amplitude is checkpointed labeled as normalized. Cosmetic metadata inconsistency; downstream behavior is unaffected.
Verdict: Approve — The diff is well-structured, documented, internally consistent, and covered by registered tests; the findings are non-blocking (single-mode restart works, and the version-guard and multi-mode gaps are limitations rather than regressions).
Closes #294
What changed
Validation
ctest --test-dir build-ql290 --output-on-failure: 60/60 passedStacked on #303.