Issue #293: retain periodic multi-mode feedback state - #303
Conversation
krystophny
left a comment
There was a problem hiding this comment.
Review verdict: Approve
Summary:
This PR adds per-mode metadata retention to the QL-Balance KIM wave-code adapter. It introduces four module-level arrays (kim_mode_m, kim_mode_n, kim_mode_status, kim_mode_resonance), exposed via public, which are deallocated/reallocated and re-initialized inside kim_run_for_all_modes on every call and then filled in the per-mode solve loop (kim_mode_status = ierr, kim_mode_resonance = res%r_resonance). The test extends test_kim_adapter.f90 to check signed mode identity/status retention, and a benchmark doc note is added. The change is small, self-consistent, and consistent with the existing kim_periodic_scale_status/kim_periodic_current_unit pattern; the re-allocation on every call genuinely prevents stale state from a prior mode batch.
Findings:
- [minor] QL-Balance/src/test/test_kim_adapter.f90:230-233 — The new resonance-metadata branch is dead code in the test. The test sets
kim_run_type = "electromagnetic"(line 210), sotrim(kim_run_type) == 'electrostatic_periodic'is always false and thekim_mode_resonance(1) <= 0.0d0FAIL check is never reached. Worse,r_resonanceis only ever populated by the periodic electrostatic solver (poisson_periodic.f90setsEBdat%r_resonance); the electromagnetic solver never sets it, sokim_mode_resonanceis always 0 on this path. The PR's headline feature (resonance metadata for periodic multi-mode feedback) is therefore not actually exercised by the test. Consider adding a periodic-run variant (or a second test withkim_run_type = 'electrostatic_periodic') that assertskim_mode_resonance(1) > 0. - [minor] docs/benchmarks/2026-08-12-kim-293-multimode-feedback.md — The claim "A failed or nonresonant mode retains a nonzero status and is reported by the adapter" is inaccurate. On a failed solve (
ierr /= KIM_OK)kim_run_for_all_modescallsstop 1immediately (kim_wave_code_adapter.f90:387-389), so a nonzero status is never retained/reported downstream. Andkim_mode_statusis only set from the solver return code, not from resonance, so a nonresonant mode keeps status 0 with onlykim_mode_resonance ≤ 0. The doc overstates what the code actually guarantees. - [minor] QL-Balance/src/base/kim_wave_code_adapter.f90:391 —
kim_mode_resonance(i_mn) = res%r_resonanceis read unconditionally even for non-periodic runs, wherer_resonancestays 0. Harmless (it just stores 0 metadata), but the value is meaningless outside the periodic path; a comment or guard (if (periodic) ...) would make intent clearer. Not blocking.
Verdict: Approve — the adapter change is correct, well-scoped, and consistent with existing per-mode storage patterns; the findings are minor (an untested resonance branch, a doc overstatement, and a harmless unconditional read) and don't block merge.
…ormalization' into HEAD # Conflicts: # QL-Balance/src/base/kim_wave_code_adapter.f90
krystophny
left a comment
There was a problem hiding this comment.
Review verdict: Request changes
Summary: This PR (KIM #293) makes the QL-Balance↔KIM periodic adapter retain per-mode metadata (signed m/n, solve status, resonant radius), relaxes the periodic-embedding grid validation to support KIM's endpoint-exclusive periodic Fourier grid, renames KIM's prepare_resonances to kim_prepare_resonances (avoiding a symbol clash with QL-Balance's own gengrid.f90 routine now that both link into the QL-Balance binary), and reworks test_kim_adapter to run in electrostatic_periodic mode. The rename is complete and consistent; the validation relaxation is sound and well covered by the expanded test_periodic_embedding; the new metadata arrays are additive and cleanly deallocated/reallocated per batch. My main concern is the test's density-rescaling assertion, which I believe no longer measures what it claims in periodic mode.
Findings:
-
[major]
QL-Balance/src/test/test_kim_adapter.f90:259-269— the "rescaled density changed KIM Br" assertion was switched fromelectromagnetictoelectrostatic_periodic(kim_run_type = "electrostatic_periodic", line 211), but in periodic mode KIM hardcodesEBdat%Br = Br_const(a constant boundary value1.0+0i) inKIM/src/electrostatic_poisson/poisson_periodic.f90:352,378. The embeddedkim_Br_modes(:,1)is therefore just that constant inside the compact window and zero outside. The test varies only density (params_b(1,:)from1.6*base_ntobase_n, lines 223/259), leaving Te/Ti/B fixed, so the window geometry — derived from the Larmor radiusrho_L(rm)inpoisson_periodic.f90:318-321— and the Br profile are unchanged.response_change = maxval(abs(kim_Br_modes(:,1) - br_rescaled))then evaluates to ~0 and the check hits theFAIL: KIM Br remained frozenbranch. At best the assertion is vacuous (it can only pass if the embedding window shifts with density, which rho_L does not); at worst it breaks CI. Required: verify against CI, and either keepelectromagneticfor this assertion (periodic coverage is already provided by the new metadata check) or assert on a periodic quantity that genuinely depends on density (e.g., phi/jpar amplitudes) rather than the constant Br. -
[minor]
KIM/src/diagnostics/kim_diagnostics_mod.f90:66— the comment still reads "prepare_resonances leaves r_res = 0" after the subroutine was renamed tokim_prepare_resonances. Update the comment to the new name.
No findings on the validate_embedding_grids relaxation (periodic_embedding_m.f90): the new per-global-point weighted-coverage check is correct, the smooth ramp is exactly zero at the support endpoints so no weighted point is skipped, and test_periodic_embedding.f90 exercises the endpoint-exclusive case. The CMake profile copies are harmless (profiles are injected in memory via kim_profiles_from_balance).
Verdict: Request changes — the switched run type likely makes the pre-existing density-rescaling assertion fail (or vacuous) in test_kim_adapter, so CI should be verified and the assertion re-targeted before merge; the stale comment should also be corrected.
Closes #293.
Retains signed per-mode identity, solve status, resonance radius, transition state, current unit response, and normalization amplitude across periodic KIM batches. The adapter resets response storage before each refresh, receives updated QL profiles, and preserves independent mode tensors for incoherent global accumulation.
Extended adapter regression covers signed identity/status and repeated profile refresh.
Validation: test_kim_adapter passed; ql-balance_lib builds.