Test - TEST - Decomposition invariance of the unified Riccati Delta-prime path - #362
Open
d-burg wants to merge 5 commits into
Open
Test - TEST - Decomposition invariance of the unified Riccati Delta-prime path#362d-burg wants to merge 5 commits into
d-burg wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens the ForceFreeStates parallel FM/BVP verification pipeline by adding explicit thread-invariance tests, updating CI to exercise multi-threaded execution, and aligning documentation/tests with the actual chunk-balancing formula used for the parallel BVP path.
Changes:
- Add
runtests_thread_invariance.jlto assert parallel BVP results are bit-identical betweenparallel_threads=1and2when running multi-threaded. - Add a multi-threaded CI matrix leg (
JULIA_NUM_THREADS=4) without renaming the existing branch-protection-required job contexts. - Update the
balance_integration_chunksdocstring and the parallel-integration test to reflect the effective thread cap logic.
Note: Project policy still requires third-party human review before merge.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/runtests.jl |
Adds the new thread-invariance test file to the default test suite. |
test/runtests_thread_invariance.jl |
New tests to verify thread invariance of Δ′/et across parallel_threads caps. |
test/runtests_parallel_integration.jl |
Fixes stale formula in the test to use effective thread cap, matching implementation intent. |
src/ForceFreeStates/EulerLagrange.jl |
Updates balance_integration_chunks docstring to describe current targeting behavior (but needs a small alignment fix). |
CLAUDE.md |
Corrects documented test invocation and updates the list of example test files. |
.github/workflows/test.yaml |
Adds a multi-threaded test leg by setting JULIA_NUM_THREADS via a matrix include. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+103
to
+106
| for j in 1:serial.msing | ||
| @test threaded.dpm[j, j] === serial.dpm[j, j] | ||
| end | ||
| @test threaded.dpm == serial.dpm |
Comment on lines
+80
to
+82
| if Threads.nthreads() < 2 | ||
| @info "Thread-invariance tests skipped: effective_threads collapses to 1 in a single-threaded session. Run with `julia -t 4` (CI covers this in its multi-threaded matrix leg)." | ||
| @test true |
Comment on lines
+70
to
+71
| `max(2*msing + 3, 4*effective_threads, 8*(msing + 1) + msing)`, where | ||
| `effective_threads = min(Threads.nthreads(), ctrl.parallel_threads)`. |
d-burg
enabled auto-merge
August 13, 2026 23:12
d-burg
force-pushed
the
feature/thread-invariance
branch
from
August 14, 2026 16:30
0440553 to
6b9c39b
Compare
Collaborator
|
@d-burg assign someone to review or mark as draft |
matt-pharr
marked this pull request as draft
August 15, 2026 20:10
auto-merge was automatically disabled
August 15, 2026 20:10
Pull request was converted to draft
Collaborator
|
@d-burg you will be interested in the chunking/threading changes for riccati I made that are now on develop. Should make it always invariant to thread count. |
d-burg
force-pushed
the
feature/thread-invariance
branch
2 times, most recently
from
August 15, 2026 21:23
68cce32 to
af5474d
Compare
d-burg
changed the title
Collaborator
|
et[1] drifting mildly in the decomposition is acceptable IMO, but if we decide otherwise we should be able to fix the order things are summed in to fix it. You can look at it here if you want but @logan-nc can decide whether he thinks that is an issue or not, I don't think so personally. |
Collaborator
|
That small of a drift seems ok to me? Maybe @adrianaghiozzi knows better if its a red flag or benign/expected. |
d-burg
force-pushed
the
feature/thread-invariance
branch
from
August 16, 2026 23:01
af5474d to
c61678b
Compare
d-burg
changed the title
d-burg
force-pushed
the
feature/thread-invariance
branch
from
August 18, 2026 15:50
e9a7af7 to
73a5db7
Compare
d-burg
marked this pull request as ready for review
August 18, 2026 17:25
ForceFreeStatesControl documents that the parallel FM/BVP path produces bit-identical
Delta' across thread counts, but nothing tested it by varying threads, and the example
decks disagreed about whether to rely on it: the SLAYER deck pins parallel_threads = 1
"to keep the regression Delta' (and hence gamma) reproducible", while the DIII-D-like
ideal deck — whose Delta' the regression harness tracks — runs parallel_threads = 2.
A golden Delta' has to be a property of the physics rather than of the machine it was
measured on, so this settles the question by measurement.
Measured on the DIII-D-like deck at parallel_threads = 1, 2, 4 and 16: the Delta' matrix
diagonal and et[1] are bit-identical throughout. The 16-thread configuration is the
informative one — it lifts 4*effective_threads above the min_bvp_intervals floor and so
produces a genuinely different decomposition (64 chunks vs 53, with different boundaries),
which reassociates the propagator products without moving the result. Invariance therefore
holds across decomposition, not merely across scheduling.
- test/runtests_thread_invariance.jl compares parallel_threads 1 vs 2 on the Solovev
and DIII-D-like decks, asserting exact equality rather than a tolerance: the code
claims bit-identity, and a tolerance would mask the reassociation the test exists to
catch. It also asserts the chunk boundaries are unchanged at these caps, so that a
future decomposition change surfaces as a failure instead of silently turning the
comparison into a stronger claim than intended. Skipped when the session has one
thread, where effective_threads collapses to 1 and every comparison is vacuous.
- The test workflow gains a multi-threaded leg (JULIA_NUM_THREADS = 4). The suite had
only ever run single-threaded, so the parallel paths were exercised solely in their
degenerate form. Existing job names are preserved byte-identically because branch
protection names them as required checks.
- balance_integration_chunks' docstring gave target_n as
max(2*msing + 3, 4*Threads.nthreads()), omitting both the parallel_threads cap and the
min_bvp_intervals term that actually dominates. runtests_parallel_integration.jl
mirrored the same stale formula and would fail on a machine with more threads than the
cap; it passed only because CI is single-threaded.
- CLAUDE.md's single-test-file invocation could not work (runtests.jl passes ARGS to
include, which resolves relative to test/), and two of the listed files do not exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
develop added a guard requiring intr.nlow/nhigh to be assigned before sing_lim!, since set_psilim_via_dmlim truncates at (last_rational_q + dmlim)/n and so needs n, and fixed the ordering in runtests_parallel_integration.jl. This test file copied the old ordering from that file before the guard existed; the rebase carried it forward because the two files never overlap textually, so git merged them cleanly while the semantics diverged. Only the multi-threaded CI leg surfaced it: the Solovev testset does not truncate via dmlim, and the single-threaded legs skip the whole testset because effective_threads collapses to 1 there. No thread-invariance claim is affected — the failure was an exception during setup, not a comparison mismatch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Δ′ path The riccati unification made the original thread-invariance test obsolete: it varied parallel_threads, which no longer exists, and the chunk target is now derived from msing alone so thread-count independence is structural and unit-tested upstream. What those unit tests cannot assert is the end-to-end claim: that cutting the same integration into a genuinely different set of chunks — which reassociates the fundamental-matrix products — leaves the physics output unchanged. This test pins that, steering the decomposition directly via nchunks (auto vs auto+11, boundaries verified different), so it is meaningful at any thread count. Measured on the DIII-D-like deck under the unified driver: the Δ′ matrix is bit-identical across decompositions (asserted with ===; a tolerance would hide exactly the reassociation drift the test exists to catch), but et[1] is not — it drifts by 2.7e-8 relative, where the pre-unification driver was exact. That sensitivity is recorded honestly rather than hidden: @test_broken on exactness (an Unexpected Pass will force the strict assertion back if a driver change restores it) plus a documented 1e-6 ceiling to catch it growing by orders of magnitude. The multi-threaded CI leg is kept: the chunk loop runs through Threads.@threads, which executes serially in a single-threaded session, so without this leg no CI job ever exercises concurrent scheduling. The CLAUDE.md single-test-file invocation fix is kept (runtests.jl passes ARGS to include, which resolves relative to test/). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The multi-threaded matrix leg added JULIA_NUM_THREADS in a second `env:` key alongside the existing DEPOT_PATHS one, so the test job carried a duplicate YAML mapping key. GitHub rejects the whole workflow file on that, which is why no test run reported on this branch and the required Tests context could never be satisfied. Had it parsed, the later block would have won and dropped JULIA_NUM_THREADS entirely, leaving the new leg single-threaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-burg
force-pushed
the
feature/thread-invariance
branch
from
August 18, 2026 17:48
73a5db7 to
3c5ba1f
Compare
Collaborator
|
@d-burg this is currently marked as open, is this ready for review? |
d-burg
marked this pull request as draft
August 19, 2026 13:32
… API The test hand-assembled the stage sequence -- sing_lim!, sing_find!, manual mlow/mhigh arithmetic, make_metric, make_matrix, eulerlagrange_integration, free_run, compute_delta_prime_matrix! -- roughly 30 internal calls reproducing the orchestration main() happened to use. That is a copy of the system under test, and it had already drifted: the hand-rolled mode range gives et[1] = 0.801371 where the production path gives 0.804101, a 0.34% difference, so the test was pinning a configuration GPEC never runs. Now posed as EulerLagrangeProblem + solve(prob, Riccati(; nchunks)), which #393 published, so the test exercises the same path production does. Rewriting it also scoped the claim. Invariance holds at or above the msing-derived chunk target -- auto(53) vs 64 is bit-identical -- but not below it: auto(53) vs 29 moves Delta-prime by ~1e-6 relative. Requesting fewer chunks than the floor is a structurally deficient decomposition rather than merely a different one, since the floor exists to give the surface crossings room. The old test only ever probed the more-chunks direction and so never saw this. The et[1] assertions are gone. et[1] drift across decompositions is a value changing over time, which the regression harness tracks far better than a @test_broken that reports as expected-and-fine forever plus an isapprox 30x looser than the measured effect. It survives here only as a witness that the two runs really were different computations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-burg
marked this pull request as ready for review
August 19, 2026 21:33
Collaborator
Author
|
Ready now, thanks @matt-pharr ! |
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
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.
Release note
Adds a regression test asserting that the Riccati Δ′ matrix is bit-identical when the same
integration is cut into a genuinely different set of chunks, and runs the CI suite multi-threaded so
the chunked propagator path is actually exercised concurrently.
Notes for reviewers
Rewritten after #381 (riccati unification), which superseded this PR's original form: it varied
parallel_threads, which #381 deleted, and #381 made the chunk target structurally thread-independent (derived frommsingalone) with unit tests pinning the thread-free target and bit-identical boundaries.What this now adds
The end-to-end claim the #381 unit tests cannot make: cutting the same integration into a genuinely different chunk decomposition — which reassociates the fundamental-matrix products,
((A·B)·C)·D→(A·B)·(C·D)— leaves the physics output unchanged. The test steers the decomposition directly vianchunks(auto vs auto+11, boundaries asserted different first, so the equality can never pass vacuously) and therefore runs meaningfully at any thread count.A multi-threaded CI leg (
JULIA_NUM_THREADS: 4). The chunk loop executes throughThreads.@threads, which runs serially in a single-threaded session — and the existing matrix legs are single-threaded, so no CI job has ever exercised concurrent scheduling. Existing job names are preserved byte-identically (branch protection names them; see #360).CLAUDE.md fix: the documented single-test-file invocation (
test/runtests.jl test/runtests_foo.jl) cannot work —runtests.jlpasses ARGS toinclude, which resolves relative totest/. Corrected, plus the multi-threaded invocation documented.et[1]is not decomposition-invariant under the unified driverMeasured on the DIII-D-like ideal deck, auto (53 chunks) vs auto+11:
===)et[1]The test records this honestly rather than hiding it:
@test_brokenonet[1]exactness (a future driver change restoring exactness reports an Unexpected Pass, forcing the strict assertion back), plus a documentedrtol=1e-6ceiling — 30× the measured effect, there to catch the sensitivity growing by orders of magnitude, explicitly not chosen to make anything pass. If this is a known/accepted property of the serial Riccati-crossing assembly, the@test_brokenline is the right permanent record; if not, it may deserve an issue.Verification
julia -t 4: 11 pass, 1 broken (the recordedet[1]exactness), 60.9 s.🤖 Generated with Claude Code