Skip to content

Issue #293: retain periodic multi-mode feedback state - #303

Open
marjohma wants to merge 4 commits into
feature/kim-292-target-current-normalizationfrom
feature/kim-293-multimode-profile-feedback
Open

Issue #293: retain periodic multi-mode feedback state#303
marjohma wants to merge 4 commits into
feature/kim-292-target-current-normalizationfrom
feature/kim-293-multimode-profile-feedback

Conversation

@marjohma

Copy link
Copy Markdown
Member

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.

@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:
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:

  1. [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), so trim(kim_run_type) == 'electrostatic_periodic' is always false and the kim_mode_resonance(1) <= 0.0d0 FAIL check is never reached. Worse, r_resonance is only ever populated by the periodic electrostatic solver (poisson_periodic.f90 sets EBdat%r_resonance); the electromagnetic solver never sets it, so kim_mode_resonance is 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 with kim_run_type = 'electrostatic_periodic') that asserts kim_mode_resonance(1) > 0.
  2. [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_modes calls stop 1 immediately (kim_wave_code_adapter.f90:387-389), so a nonzero status is never retained/reported downstream. And kim_mode_status is only set from the solver return code, not from resonance, so a nonresonant mode keeps status 0 with only kim_mode_resonance ≤ 0. The doc overstates what the code actually guarantees.
  3. [minor] QL-Balance/src/base/kim_wave_code_adapter.f90:391 — kim_mode_resonance(i_mn) = res%r_resonance is read unconditionally even for non-periodic runs, where r_resonance stays 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.

@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 (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:

  1. [major] QL-Balance/src/test/test_kim_adapter.f90:259-269 — the "rescaled density changed KIM Br" assertion was switched from electromagnetic to electrostatic_periodic (kim_run_type = "electrostatic_periodic", line 211), but in periodic mode KIM hardcodes EBdat%Br = Br_const (a constant boundary value 1.0+0i) in KIM/src/electrostatic_poisson/poisson_periodic.f90:352,378. The embedded kim_Br_modes(:,1) is therefore just that constant inside the compact window and zero outside. The test varies only density (params_b(1,:) from 1.6*base_n to base_n, lines 223/259), leaving Te/Ti/B fixed, so the window geometry — derived from the Larmor radius rho_L(rm) in poisson_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 the FAIL: KIM Br remained frozen branch. 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 keep electromagnetic for 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.

  2. [minor] KIM/src/diagnostics/kim_diagnostics_mod.f90:66 — the comment still reads "prepare_resonances leaves r_res = 0" after the subroutine was renamed to kim_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.

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