Trace local Lorentz constant state - #163
Conversation
There was a problem hiding this comment.
Review verdict: Approve
I have thoroughly reviewed the diff, the surrounding code, the test file, and the caller integration. Here is my review.
Summary: PR #163 adds "constant state" null-space diagnostic tracing to the Lorentz projection diagnostics module, with routines to assemble a reference constant solution, compute sparse-matrix residuals, and write trace files. Tests and the QL caller integration are updated to exercise the new routines.
Findings:
- minor
lorentz_projection_diagnostics_mod.f90:370-388—decode_state_index's search loopdo while (step < iend)exits whenstep == iend, leavingstepat the last step. This is the correct result, but the loop semantics are non-obvious; a brief comment noting thatstepis intentionally left at the last matching step would aid maintainability. - minor
lorentz_projection_diagnostic_mod.f90:318-368—record_local_constant_rowaccessesbhat(column_step)after decoding. Ifdecode_state_indexreturns an out-of-rangestep(e.g., for an invalid index that passes theierr=0check but yieldsstepoutsidebhatbounds), this would be an out-of-bounds access. The current validation guards againstindex < 0andibeg > iend, but not againststepbeing outsidebhat's declared bounds. Consider adding a bounds check onstepbefore using it to indexbhat.
Verdict: Approve — The change is well-structured with comprehensive input validation, thorough tests covering all new public routines, and properly guarded integration into ripple_solver_axi_test.f90 behind the isw_lorentz.EQ.1 guard. The minor items are suggestions for robustness, not blockers.
There was a problem hiding this comment.
Review verdict: Comment
Summary
PR #163 adds optional diagnostic tracing for the Lorentz constant-state projection in the QL ripple solver, exposed through a new lorentz_projection_diagnostics_mod API and gated by the NEO2_LOCAL_PROJECTION_TRACE_FILE environment variable. The diff is additive (331 lines across three files), with caller integration in ripple_solver_axi_test.f90 and test coverage in TEST/test_lorentz_projection_diagnostic.f90.
Findings
-
major
NEO-2-QL/ripple_solver_axi_test.f90(inline decode block) — The caller duplicatesdecode_state_indexlogic with an inlineDO WHILE(constant_sparse_index...)block instead of calling the already-publicdecode_state_indexsubroutine from the diagnostics module. This risks divergence between the two implementations and defeats the purpose of extractingdecode_state_indexinto a reusable routine. Required fix: replace the inline decode block with a call todecode_state_index, or document why the inline copy is necessary. -
minor
NEO-2-QL/lorentz_projection_diagnostics_mod.f90(initialize_output) —initialize_outputopens a file withopen(newunit=iunit, ...)but the unit is local; subsequentrecord_*routines callinitialize_outputagain and open independently. If the file is opened multiple times or the unit is lost, traces may be interleaved or truncated. Consider storing the unit in a module-saved variable and guarding withrow_output_initialized/output_initializedconsistently. -
minor
TEST/test_lorentz_projection_diagnostic.f90— The test exercisesassemble_local_constant_stateandcompute_sparse_constant_residualwith small fixed arrays, but does not verify thedecode_state_indexdecoding against known (step, laguerre, sigma, band) tuples. Since the caller now relies on this decoding (see finding 1), a direct unit test ofdecode_state_indexwould close the gap.
Verdict
Comment — The diagnostic functionality is reasonable and opt-in, but the inline decode duplication in the caller should be resolved (or justified) before merge to avoid maintenance divergence.
Summary
Add an opt-in stage trace that evaluates the exact pitch-independent Lorentz state at three points in every single-ripple propagator:
This PR is stacked on #162. It is the minimum measurement needed to localize the finite-resolution constant-distribution transport defect before changing the solver.
The internal state uses the code's documented unknown
[
Y_k=\int_{\eta_{k-1}}^{\eta_k}f,d\eta,
]
including the moving final-band width (1/B-\eta_N) and the reversed storage order of the counter-passing branch. The sparse right-hand side applies those exact widths only at the incoming co-passing and counter-passing boundaries. The matrix residual and solved-state error are appended to
NEO2_LOCAL_PROJECTION_TRACE_FILE; with that variable unset, the new path returns before allocation and does not call the extra solve.Decision rule
No correction is applied in this PR. The measured stage will determine the smallest justified fix.
Preserved invariants
Verification
Test fails on main
Main has no executable check for the exact internal constant state or its sparse residual:
Test passes after fix
Diagnosis and successor
The stage trace localized the defect to a topology crossing, not the sparse
factorization, boundary-map extraction, pitch transfer, joining, or the Lorentz
collision stencil. In propagator 192, the number of passing bands falls from 27
to 26 between spatial steps 679 and 680. The old placement routine does not
align the crossing sample when that sample is closer than both neighbours. The
constant-distribution widths on the two sides then differ by
3.365342566301611e-4; multiplication by the streaming coefficient producesthe complete
5.1395606791432513e-2sparse row residual. Collision terms cancelon the same row.
PR #164 supplies the stacked correction and regression. At its pinned head,
the worst normalized sparse constant residual is
1.0147e-16, the worst solvedconstant-state error is
3.0437e-14, and the full 199-propagator reconstructioncompletes with the same eight adaptive retries as the stacked base. A retained
stage-0 trace closes right transport at
2.2289e-14and projector intertwiningat
2.3275e-14. The failure-free campaign remains open. This PR remainsdiagnostic-only.