Skip to content

[QL-Balance] Evolve and restart periodic shielding amplitudes - #304

Open
marjohma wants to merge 5 commits into
feature/kim-293-multimode-profile-feedbackfrom
feature/kim-294-periodic-amplitude-restart
Open

[QL-Balance] Evolve and restart periodic shielding amplitudes#304
marjohma wants to merge 5 commits into
feature/kim-293-multimode-profile-feedbackfrom
feature/kim-294-periodic-amplitude-restart

Conversation

@marjohma

Copy link
Copy Markdown
Member

Closes #294

What changed

  • add per-mode accepted/trial complex periodic shielding state with current, residual, guard-status, target, relaxation, phase-policy, and normalization-version diagnostics
  • enforce the documented constant-psi unit drive on the first TimeEvolution response; subsequent refreshes recalculate target-current shielding from the latest accepted profiles
  • commit amplitude state only after an accepted profile step and restore trial state on rejected error-controlled steps
  • checkpoint accepted/trial amplitudes and normalization diagnostics in KinProfiles HDF5 snapshots and restore them on continuation, with fallback compatibility for the legacy fort.1000 layout
  • add focused rollback/commit regression coverage and implementation notes

Validation

  • complete rebuild of configured targets
  • ctest --test-dir build-ql290 --output-on-failure: 60/60 passed

Stacked on #303.

@krystophny krystophny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. [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%accepted is written to HDF5 and read back, but nothing ever reads it into kim_run_for_all_modes: drive_scale is always recomputed fresh by periodic_drive_scale (which has no memory), and the only alternative is the unit drive. Worse, on restart kim_initialize (line 127) resets periodic_constant_psi_pending = .true. after read_background_profiles_h5_timeevol restores the saved amplitude, so the first post-restart solve forces drive_scale = (1,0); sync_periodic_amplitude_trial then overwrites the restored trial with 1 and accept() 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 seed drive_scale/kim_periodic_scale_modes from the restored accepted amplitude (and clear periodic_constant_psi_pending) when state is restored, or explicitly document that the amplitude is diagnostic-only and drop the restart claim.

  2. [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 calls reject() (restoring trial to the old accepted value) but no get_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.

  3. [minor] QL-Balance/src/base/kim_wave_code_adapter.f90:415-418 — On the first (pending) solve, scale_status = 0 is hardcoded, bypassing the periodic_drive_scale guard. A degenerate unit current (below current_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.

  4. [minor] docs/benchmarks/2026-08-12-kim-294-periodic-amplitude-restart.md vs time_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 no get_dql and 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.

@krystophny krystophny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. [minor] wave_code_data_64bit.f90:686-720 (reader) vs time_evolution.f90:650-655 (writer) — periodic_normalization_version and periodic_phase_policy are checkpointed (with exposed constants intended as guard metadata), but the reader never reads or validates them; it restores amplitudes purely on the presence of periodic_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.

  2. [minor] wave_code_data_64bit.f90:606 vs writer time_evolution.f90:585/gengrid.f90:235 — The restart reader always builds f_{m_vals(1)}_{n_vals(1)}/KinProfiles/, but the writer uses h5_mode_groupname, which is "multi_mode" when numres>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 through h5_mode_groupname.

  3. [minor] kim_wave_code_adapter.f90:429-466 + time_evolution.f90:284-305 — Because kim_periodic_normalization_relaxation is hardcoded to 1.0 and every doStep re-runs get_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.

  4. [minor] kim_wave_code_adapter.f90:438-442 — In the constant-ψ branch, when scale_status == 0, drive_scale is forced to unit (1,0) but kim_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).

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