Skip to content

FFS - BUGFIX - Zero-extent ca_left/ca_right when unpopulated (issue #366) - #368

Merged
priyanshlunia merged 9 commits into
developfrom
bugfix/366-ca-uninitialized
Aug 21, 2026
Merged

FFS - BUGFIX - Zero-extent ca_left/ca_right when unpopulated (issue #366)#368
priyanshlunia merged 9 commits into
developfrom
bugfix/366-ca-uninitialized

Conversation

@logan-nc

@logan-nclogan-nc commented Aug 13, 2026•
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Fixes#366: SingularSurfaces/ca_left/ca_right were written to gpec.h5 unconditionally, but only ideal singular-surface crossings populate odet.ca_l/ca_r (allocated undef). Kinetic runs dumped uninitialized heap memory — non-reproducible output (238 vs 100 NaNs between identical runs, subnormal garbage values). Third PR in the #226 stack: base is refactor/hdf5-metadata (#364); merge order #363 → #364 → this.

Scope extension found during implementation: the galerkin-matched path (gal_matched_odestate builds an OdeState and never crosses a surface) emitted the same garbage, and benchmarks/gal_validation/gal_{epsilon,beta}_scan.jl were already consuming it. The fix covers both paths.

Fix (issue options 1 + 2)

  • ca_l/ca_r allocated with zeros instead of undef — kills in-memory nondeterminism at the root.
  • New OdeState.ca_populated::Bool (mirrors the existing du_store_populated pattern), set by the two ideal crossing routines (EulerLagrange + Riccati) and carried through the Riccati dense-ξ save/restore; galerkin-matched states keep the false default.
  • The writer emits rank-4 zero-extent sentinels when unpopulated. Datasets stay always-present — no reader KeyErrors, PR B's metadata annotations unchanged. This matches the empty-sentinel idiom already used by the FreeBoundaryStability block and the zero-extent choice ForceFreeStates - IMPROVEMENT - Compute solution derivatives on demand #365 made for its analogous unpopulated u_store case (that PR's body independently documents this bug as follow-up — cross-linking).
  • Reader guarded: Analysis.plot_delta_prime returns a placeholder on empty ca (previously indexed into it). (This PR originally also guarded the two gal-validation scans; develop has since untracked those benchmark scripts, so the stack merge resolved them as deleted and those hunks are gone.)
  • Conventions: zero-extent-array = not-computed sentinel codified in docs/development/hdf5-conventions.md; stale docs/src/stability.md claim (SingularCoupling "reads ca_l/ca_r directly" — it doesn't) corrected.

Verification

  • Determinism (the issue's own signal): two identical runs of the kinetic Solovev deck now produce byte-identical, empty ca_left/ca_right (previously differing NaN counts).
  • Tests: kinetic fullrun asserts both datasets empty; ideal schema run asserts populated + all-finite; full test/runtests.jl suite (57 testsets) passes with zero failures; docs build clean.
  • Regression harness (mandatory): all 12 cases --refs develop,local — every tracked quantity OK at 0.0e+00 (same 4 pre-existing N/As in gal_resistive_pe). New solovev_n1 quantity ca_left (checksum) locks bitwise reproducibility going forward; it reports N/A on the develop side only because develop's cached extraction predates the quantity — the local checksum is now the baseline.

Coordination

⚠️⚠️ NO MERGE WITHOUT THIRD-PARTY HUMAN REVIEW ⚠️⚠️

This PR must not be merged until a third-party human reviewer has approved it — non-negotiable project policy. Merge order: #363 → #364 → this.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz

@logan-nc@claude
FFS - BUGFIX - Emit zero-extent ca_left/ca_right when unpopulated
260a38a
SingularSurfaces/ca_left and ca_right were written to gpec.h5 unconditionally,
but only ideal singular-surface crossings populate odet.ca_l/ca_r — kinetic and
galerkin-matched runs dumped uninitialized heap memory (non-reproducible NaN
counts between identical runs; subnormal garbage values).
- Allocate ca_l/ca_r with zeros instead of undef (kills in-memory
nondeterminism at the root).
- New OdeState.ca_populated flag (mirrors du_store_populated), set by the two
ideal crossing routines (EulerLagrange + Riccati) and carried through the
Riccati dense-xi save/restore; galerkin-matched OdeStates keep the default
false.
- The writer emits rank-4 zero-extent sentinels when the flag is false —
datasets stay always-present (no reader KeyErrors, metadata annotations
unchanged), matching the established empty-sentinel idiom.
- Guard the two readers that index ca: Analysis.plot_delta_prime returns a
placeholder on empty ca, and the gal_{epsilon,beta}_scan benchmarks record
NaN for the ca-jump diagnostic (they were consuming garbage on galerkin runs
already); also fix a leftover legacy haskey(f["singular"], ...) in both.
- Zero-extent not-computed sentinel codified in hdf5-conventions.md; stale
stability.md claim about SingularCoupling reading ca_l/ca_r corrected.
- Tests: kinetic fullrun asserts empty ca datasets; ideal schema run asserts
populated + finite. New solovev_n1 harness quantity checksums ca_left to
lock bitwise reproducibility.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz
This was referenced Aug 13, 2026
Closed
Merged
This was referenced Aug 14, 2026
Merged
Open
Merged
logan-ncand others added 4 commits August 14, 2026 16:43
@logan-nc@claude
Merge refactor/hdf5-metadata into bugfix/366-ca-uninitialized
39c5e5d
Brings the remediated stack base up (second develop sync, review fixes,
reviewer dispositions, Tearing metadata remediation). Two modify/delete
conflicts: benchmarks/gal_validation/gal_{beta,epsilon}_scan.jl were untracked
on develop while this branch had guarded their ca-jump reads — accepted the
deletion; those guard hunks are moot for files that no longer exist.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz
@logan-nc
Merge branch 'refactor/hdf5-metadata' into bugfix/366-ca-uninitialized
4b6bdd2
@logan-nc@claude
Merge refactor/hdf5-metadata into bugfix/366-ca-uninitialized
9e9578d
Brings the second review round (native complex storage, dataset naming
standardization) up the stack. Two content conflicts, both this branch's
zero-extent ca sentinel meeting the base's renames: resolved as the renamed
paths (rational_count/rational_psi/rational_q/dqdpsi) with the ca_populated
sentinel writes and the zero-extent long_name notes preserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz
@logan-nc
Merge remote-tracking branch 'origin/refactor/hdf5-metadata' into bug…
2f00139
…fix/366-ca-uninitialized
# Conflicts:
#	src/HDF5Schema.jl
Base automatically changed from refactor/hdf5-metadata to developAugust 15, 2026 13:32
@logan-nclogan-nc added the bug Something isn't working label Aug 15, 2026
logan-nc
logan-nc commented Aug 15, 2026
View reviewed changes
Comment threaddocs/src/stability.md
@logan-nc@claude
DOCS - FIX - Capitalize the Delta_prime_matrix HDF5 path in stability.md
809366d
The note added by this PR cited the dataset as
SingularSurfaces/delta_prime_matrix; the literature-capitalization rule
introduced with the schema overhaul writes it as Delta_prime_matrix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGmFuAw5JdYrAyBSXCVssR
@logan-nc

logan-nc commented Aug 15, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@matt-pharr I assume this will be also fixed in your abstraction of integrators but lets just merge the fix here for now - it'll be a minor conflict compared to the rest you have

@matt-pharrmatt-pharr mentioned this pull request Aug 15, 2026
Open
@logan-nclogan-nc mentioned this pull request Aug 19, 2026
Merged
@logan-nc
logan-nc removed the request for review from matt-pharrAugust 20, 2026 14:50
@logan-nc

logan-nc commented Aug 20, 2026

Copy link
Copy Markdown
CollaboratorAuthor

@priyanshlunia review & merge this in the next 24 hrs please. Just fix any minor issues yourself directly. Ping me if anything significant changes.

@priyanshlunia@claude
Merge origin/develop into bugfix/366-ca-uninitialized
7bc0c03
Resolved against develop's post-#365 writer: ca_left/ca_right now guard on
(diag !== nothing && diag.ca_populated) with rank-4 zero-extent sentinels;
dropped the ca_populated save/restore hunks in the deleted
_populate_dense_xi_via_serial_el!. Also repaired conflict markers committed
to develop in docs/development/regression-harness.md (kept both sides,
dropped the stale 'thread counts not forced' paragraph superseded by -t auto).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added the bugfix Something was wrong and now is not label Aug 21, 2026
@priyanshlunia

priyanshlunia commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Regression harness report — PR #368 post-merge verification

Command:julia --project=regression-harness regression-harness/regress.jl --refs develop,local
Local ref: merge commit 7bc0c033 (bugfix/366-ca-uninitialized with origin/develop @ 9be3ac58 merged in, conflicts resolved)
Environment (both refs): julia 1.12.2, arm64-apple-darwin24.0.0, manifest 5e468ac3 (pinned), 6 threads / 6 BLAS

Result: all 14 cases ran successfully on both refs — no tracked quantity changed

CaseQuantitiesStatus
diiid_n147all OK, 0.0e+00
diiid_n1_riccati17all OK, 0.0e+00
diiid_slayer_n115all OK (root-finder diffs ≤ 3e-6, well under per-quantity noise thresholds)
efit_fixedbdy_separatrix5all OK, 0.0e+00
gal_resistive_diiid10all OK, 0.0e+00
gal_resistive_pe8N/A on both refs (pre-existing, see note)
ggj_ray_q500i4all OK, 0.0e+00
ggj_reference4all OK, 0.0e+00
solovev_kinetic_calculated14all OK, 0.0e+00
solovev_kinetic_multiion6all OK, 0.0e+00
solovev_kinetic_ntv6all OK, 0.0e+00
solovev_kinetic_nuzero14all OK, 0.0e+00
solovev_multi_n15all OK, 0.0e+00
solovev_n122all OK, 0.0e+00 — includes the new ca_left (checksum) quantity

Key verifications for this PR

  • solovev_n1ca_left (checksum): byte-identical on develop and local
    (6673eae3dce0afad7b32e19130842592ca7e773d59044eefa383cbf4a5c1f003) — the ideal-path
    asymptotic coefficients are untouched by the fix and now bitwise-locked going forward.
  • End-to-end sentinel check on the merged tree's harness outputs (h5ls on gpec.h5):
    • Kinetic (Solovev_kinetic_calculated_example): ca_left/ca_right = {0, 0, 0, 0} (zero-extent) ✓
    • Galerkin-matched (DIIID-like_gal_resistive_pe_example): {0, 0, 0, 0} (zero-extent) ✓
    • Ideal Riccati (DIIID-like_riccati_deltaprime_example): {5, 2, 35, 35} (populated, all compared OK) ✓

Notes

  • The 8 gal_resistive_pe N/As are datasets missing from gpec.h5 on both refs — a
    pre-existing develop-side issue unrelated to this PR (every tracked quantity in that case
    except runtime currently resolves to missing; the case TOMLs likely need a refresh after
    the recent schema work — suggested as a follow-up issue).
  • SLAYER (diiid_slayer_n1) root quantities differ at the 1e-6..1e-3 level, far below their
    configured noise thresholds (e.g. slayer_Q: max diff ~3e-6 vs threshold 1e-4) — expected
    AMR/root-finder noise, reported OK by the harness.

@priyanshlunia
priyanshlunia merged commit 2cb4f64 into developAug 21, 2026
7 of 8 checks passed
@priyanshlunia
priyanshlunia deleted the bugfix/366-ca-uninitialized branch August 21, 2026 16:56
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@priyanshluniapriyanshluniapriyanshlunia approved these changes

Assignees

@priyanshluniapriyanshlunia

Labels

bugSomething isn't workingbugfixSomething was wrong and now is not

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

singular/ca_left and ca_right are written as uninitialized memory on kinetic runs

2 participants

@logan-nc@priyanshlunia