FFS - BUGFIX - Zero-extent ca_left/ca_right when unpopulated (issue #366) - #368
Merged
Conversation
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
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
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
…fix/366-ca-uninitialized # Conflicts: # src/HDF5Schema.jl
logan-nc
commented
Aug 15, 2026
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
Collaborator
Author
|
@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 |
Collaborator
Author
|
@priyanshlunia review & merge this in the next 24 hrs please. Just fix any minor issues yourself directly. Ping me if anything significant changes. |
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>
Collaborator
Regression harness report — PR #368 post-merge verificationCommand: Result: all 14 cases ran successfully on both refs — no tracked quantity changed
Key verifications for this PR
Notes
|
priyanshlunia
approved these changes
Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #366:
SingularSurfaces/ca_left/ca_rightwere written togpec.h5unconditionally, but only ideal singular-surface crossings populateodet.ca_l/ca_r(allocatedundef). 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 isrefactor/hdf5-metadata(#364); merge order #363 → #364 → this.Scope extension found during implementation: the galerkin-matched path (
gal_matched_odestatebuilds anOdeStateand never crosses a surface) emitted the same garbage, andbenchmarks/gal_validation/gal_{epsilon,beta}_scan.jlwere already consuming it. The fix covers both paths.Fix (issue options 1 + 2)
ca_l/ca_rallocated withzerosinstead ofundef— kills in-memory nondeterminism at the root.OdeState.ca_populated::Bool(mirrors the existingdu_store_populatedpattern), set by the two ideal crossing routines (EulerLagrange + Riccati) and carried through the Riccati dense-ξ save/restore; galerkin-matched states keep thefalsedefault.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 unpopulatedu_storecase (that PR's body independently documents this bug as follow-up — cross-linking).Analysis.plot_delta_primereturns 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.)docs/development/hdf5-conventions.md; staledocs/src/stability.mdclaim (SingularCoupling "reads ca_l/ca_r directly" — it doesn't) corrected.Verification
ca_left/ca_right(previously differing NaN counts).test/runtests.jlsuite (57 testsets) passes with zero failures; docs build clean.--refs develop,local— every tracked quantityOKat 0.0e+00 (same 4 pre-existing N/As in gal_resistive_pe). Newsolovev_n1quantityca_left (checksum)locks bitwise reproducibility going forward; it reportsN/Aon the develop side only because develop's cached extraction predates the quantity — the local checksum is now the baseline.Coordination
ForceFreeStatesStructs.jl,EulerLagrange.jl,Riccati.jl, the main writer). This diff is tiny — whichever lands second rebases trivially.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