Skip to content

ForceFreeStates - BUG FIX - Pass direction arg to IntegrationChunk in kinetic EL crossing - #336

Merged
logan-nc merged 2 commits into
developfrom
bugfix/kinetic-el-integrationchunk-arity
Jul 30, 2026
Merged

ForceFreeStates - BUG FIX - Pass direction arg to IntegrationChunk in kinetic EL crossing#336
logan-nc merged 2 commits into
developfrom
bugfix/kinetic-el-integrationchunk-arity

Conversation

@logan-nc

Copy link
Copy Markdown
Collaborator

Summary

cross_kinetic_singular_surf! constructed its placeholder IntegrationChunk with a stale 4-argument call:

IntegrationChunk(0.0, 0.0, false, 0) # (psi_start, psi_end, needs_crossing, ising)

but IntegrationChunk is an @kwdef struct whose direction field (added later for bidirectional parallel FM) makes the positional constructor require 5 arguments. The ising=0/direction=1 defaults apply only to the keyword constructor, not the positional one, so the 4-arg call throws:

MethodError: no method matching IntegrationChunk(::Float64, ::Float64, ::Bool, ::Int64)

The fix passes direction=1, matching the two sibling placeholder call sites (cross_ideal_singular_surf! and the Riccati crossing), which already use the 5-arg form.

Why it was latent

The line is only reached on the serial Euler–Lagrange shooting solver (use_parallel=false) when a kinetic singular surface is present (kinetic_factor>0, self-consistent Mode A). Until now, self-consistent kinetic runs always routed through the parallel-FM/Riccati path, which bails out earlier with "kinetic_factor > 0 not implemented yet in Riccati". So this call site was never executed and the defect stayed hidden.

Verification

  • Empirically reproduced the MethodError for the 4-arg call and confirmed the 5-arg call succeeds.
  • With the fix, a self-consistent kinetic run (use_parallel=false, kinetic_factor=1) finds and crosses the kinetic singular surface and completes end-to-end with converged output.
  • Regression harness (diiid_n1, solovev_n1): no attributable change — the touched path is unreachable in these ideal/perturbative cases (confirmed by isolation; the only residual scatter is pre-existing threaded-reduction non-determinism in near-singular PE diagnostics).

🤖 Generated with Claude Code

https://claude.ai/code/session_01SsLUydP2gJbE1tGoaHDiS1

@logan-nc

Copy link
Copy Markdown
CollaboratorAuthor

@claude review this

@claude

claudeBot commented Jul 29, 2026

Copy link
Copy Markdown

Review

Correct, minimal fix. IntegrationChunk is @kwdef with a 5th direction::Int = 1 field added later; the positional constructor at EulerLagrange.jl:679 was never updated, so it would MethodError the first time cross_kinetic_singular_surf! was actually reached (serial use_parallel=false + kinetic_factor>0). The 5-arg form now matches the two sibling call sites (cross_ideal_singular_surf!:610, Riccati.jl:1150).

A couple of observations, not blocking:

  • Minor inconsistency: the sibling call at EulerLagrange.jl:610 passes the real ising value as the 4th positional arg, while this fix keeps 0 (IntegrationChunk(0.0, 0.0, false, 0, 1)) instead of the ising::Int argument already in scope at line 670/676. I confirmed sing_der! (Sing.jl:1105) discards the chunk (ctrl, equil, ffit, intr, odet, _ = params), so this is functionally inert either way — but passing the real ising would be more consistent with the sibling call and less surprising if the chunk is ever consumed there in the future.
  • No regression/unit coverage added: per the PR description, this path (use_parallel=false with kinetic_factor>0) isn't exercised by any example config or test (use_parallel=false doesn't appear in any examples/*/gpec.toml, and there's no test referencing cross_kinetic_singular_surf!). That's presumably why this went undetected for as long as it did. Worth adding a small test/example exercising this branch (or at least a targeted unit test constructing the args and calling cross_kinetic_singular_surf!) so a future struct-field change can't silently reintroduce the same class of bug — the regression harness alone won't catch it since none of its cases hit this path.
  • Per CLAUDE.md, this PR should still go through regression-guardian before merge even though the description already reports "no attributable change" — worth confirming that was run against current develop, not a stale baseline.

Otherwise the change is exactly scoped (1 line) and matches the existing pattern used elsewhere in the file.

… kinetic EL crossing
cross_kinetic_singular_surf! built its placeholder IntegrationChunk with a stale
4-arg signature (psi_start, psi_end, needs_crossing, ising), but the struct's
@kwdef positional constructor requires the direction field added later for
bidirectional parallel FM. The 4-arg call throws MethodError. This path is only
reached on the serial Euler-Lagrange shooting solver (use_parallel=false) with a
kinetic singular surface (kinetic_factor>0), which previously errored earlier in
the Riccati branch, so the defect stayed latent. Now passes direction=1, matching
the sibling placeholder call sites in cross_ideal_singular_surf! and the Riccati
crossing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SsLUydP2gJbE1tGoaHDiS1
@logan-nc
logan-ncforce-pushed the bugfix/kinetic-el-integrationchunk-arity branch from d0ccd17 to 0059600CompareJuly 30, 2026 04:04
logan-nc added a commit that referenced this pull request Jul 30, 2026
…nChunk (PR #336 review)
Match the sibling call sites (cross_ideal_singular_surf!, Riccati crossing): pass the in-scope
ising instead of 0. Functionally inert (sing_der! discards the chunk); keeps the cherry-picked
IntegrationChunk fix in sync with the amended bugfix branch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SsLUydP2gJbE1tGoaHDiS1
@logan-nc

Copy link
Copy Markdown
CollaboratorAuthor

Addressed the review:

  • ising consistency (point 1): amended to IntegrationChunk(0.0, 0.0, false, ising, 1), passing the in-scope ising to match the sibling call sites. Still functionally inert (the chunk is discarded in sing_der!), but consistent.
  • regression-guardian (point 3): run against current develop (not a stale baseline) — diiid_n1, which is the case that exercises the kinetic FGAR quantities. No attributable change:NTV torque FGAR and dW FGAR are OK, and the rows that show a delta (delta prime ~4.5%, resonant field ~4.6–6.6%, island widths ~2.5%) are the known threaded-reduction non-determinism in near-singular PE diagnostics — identical to the develop-vs-develop baseline, not caused by this change. (The ideal diiid_n1 path never reaches cross_kinetic_singular_surf!, so the fix can't affect it.)
  • Test coverage (point 2): deferring — this is a runtime MethodError on a slow self-consistent path, so a tracked regression case would be noise rather than signal. Noted for a future lightweight guard if the placeholder call sites are revisited.

@logan-nclogan-nc added bug Something isn't working auto-merge labels Jul 30, 2026
@github-actions
github-actionsBot enabled auto-merge July 30, 2026 04:21
@logan-nclogan-nc self-assigned this Jul 30, 2026
@logan-nc
logan-nc disabled auto-merge July 30, 2026 04:46
@logan-nc
logan-nc enabled auto-merge July 30, 2026 04:47
@logan-nc
logan-nc merged commit 87f689c into developJul 30, 2026
4 checks passed
@logan-nc
logan-nc deleted the bugfix/kinetic-el-integrationchunk-arity branch July 30, 2026 05:03
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugSomething isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@logan-nc