Skip to content

Check every ensemble sample against N, not just the first - #112

Merged
michakraus merged 1 commit into
mainfrom
fix/check-size-every-ensemble-sample
Jul 30, 2026
Merged

Check every ensemble sample against N, not just the first#112
michakraus merged 1 commit into
mainfrom
fix/check-size-every-ensemble-sample

Conversation

@michakraus

Copy link
Copy Markdown
Member

Second follow-up to the review of #110, and stacked on it — it targets feature/toda-lattice-handwritten-equations rather than main, because TodaLattice._check_size only exists on that branch. The diff here is just the fix.

The other follow-up, #111, is independent and targets main.

The problem

_check_size asked _nint/_length for the size of the initial condition, and for an ensemble those report on q₀[begin] alone:

_length([q5, q7])                    # 5
_check_size(5, [q5, q7], [p5, p7])   # accepted
_check_size(5, [q5, q5], [p5, p7])   # accepted — ragged momenta too
_check_size(7, [q5, q7], [p5, p7])   # throws: "initial condition has 5 components, expected N = 7"

That last line is the shape of the bug in miniature: the assertion does fire, but it reports on sample 1 while the mismatch is in sample 2. The two-argument forms compound it, since they assert only that q₀ and p₀ hold the same number of samples, not that the samples agree in length.

What a ragged ensemble actually did

Measured, not reasoned about — a [q5, q7] ensemble declared as N = 5, integrated to t = 0.1 with ImplicitMidpoint. Both branches run to completion without complaint, and they fail differently:

Hand-written branch. The fields read N = length(q) off each state, so sample 2 is a bona fide 7-site periodic lattice:

sample sizes 5 and 7;  |sample1 − true 5-site| = 0.0;  |sample2 − true 7-site| = 0.0

The numbers are right. The defect is that you asked for a five-site ensemble and got two different physical systems, with nothing saying so — every cross-sample comparison downstream is then meaningless while looking perfectly well-behaved.

Symbolic branch — the worse one. The generated functions have N = 5 compiled in. Called directly on a seven-component state:

generated f writes 5 of 7 entries; untouched tail = [NaN, NaN]
hand-written f writes 7 of 7

Sites 6 and 7 never receive a force — in the integrator they get whatever the buffer held. The giveaway is how quietly:

|sample2 − true 7-site| = 1.48e-6

after only t = 0.1. Small because the default momentum is zero and the window is short, not because anything is right; it grows with the window. That is the failure mode that survives a casual look.

Either way, this is exactly what the assertion's own comment says it exists to prevent — "an N that disagrees with the initial condition would silently mean two different systems depending on symbolic" — surviving for samples 2…n.

The change

_check_components replaces the single length query: one method for a plain initial condition, one that walks pairs(x) for a vector of samples. Messages now name the array and the sample:

q₀ sample 2 has 7 components, expected N = 5

_nint/_length stay — the two-argument constructors still use them to infer N from the first sample, which is the right behaviour once the rest are checked against it.

Both modules are fixed together. src/linear_wave.jl had the identical first-sample-only shape; they carry the same helper by design, and fixing one alone would leave the next reader wondering which is intentional.

Tests

test/toda_lattice_tests.jl: the size testset goes 14 → 22 assertions. test/linear_wave_tests.jl: 11 → 19. Each covers a ragged q₀, a ragged p₀, both the N-form and the two-argument form, and — as the positive control — a well-formed ensemble in each of the two branches:

@test_throws AssertionError hodeensemble(M, [qM, qM1], [pM, pM])
@test_throws AssertionError hodeensemble(M, [qM, qM], [pM, pM1])
@test_throws AssertionError hodeensemble([qM, qM1], [pM, pM])       # two-argument form
@test hodeensemble(M, [qM, qM .+ 0.1], [pM, pM]) isa HODEEnsemble
@test lodeensemble(M, [qM, qM .+ 0.1], [pM, pM]; symbolic = true) isa LODEEnsemble

linear_wave_tests.jl needed HODEEnsemble/LODEEnsemble added to its imports, having previously used only the problem types.

Full Pkg.test() passes, as does the pre-push hook suite.

🤖 Generated with Claude Code

`_check_size` asked `_nint`/`_length` for the size of the initial condition,
and for an ensemble those report on `q0[begin]` alone. So a ragged ensemble was
accepted:

    _length([q5, q7])                     == 5
    _check_size(5, [q5, q7], [p5, p7])    accepted
    _check_size(5, [q5, q5], [p5, p7])    accepted   (ragged momenta too)

and the two-argument forms compounded it, since they assert only that q0 and p0
hold the same *number of samples*, not that the samples agree in length.

Neither branch then complains, and they fail differently:

  * the hand-written vector fields read the size off each state individually, so
    sample 2 is integrated as a lattice of its own size. Measured on a [q5, q7]
    ensemble over t = 0.1, sample 2 reproduces the true 7-site trajectory
    *exactly* -- the numbers are right, but the ensemble holds two different
    physical systems and nothing says so;

  * the generated functions have N baked in. Handed a 7-component state, the
    generated force writes 5 of 7 entries and leaves the last two at whatever
    the buffer held, so those sites feel no force at all. The same ensemble
    departs from the true 7-site trajectory by 1.5e-6 after t = 0.1 -- small
    enough to read as round-off, and growing with the window.

That is precisely what the assertion was added to prevent, "silently two
different systems depending on `symbolic`", surviving for samples 2..n.

`_check_components` replaces the single length query: one method for a plain
initial condition, one that walks `pairs(x)` for a vector of samples. Messages
now name the array and the sample, "q0 sample 2 has 7 components, expected
N = 5", where the old text said "initial condition has 5 components" -- reporting
on sample 1 while the mismatch was in sample 2. `_nint`/`_length` stay, since
the two-argument constructors still use them to infer N from the first sample.

Both modules are fixed together: they carry the same helper by design, and
`linear_wave.jl` had the identical first-sample-only shape.

Tests: `test/toda_lattice_tests.jl` 14 -> 22 assertions in the size testset and
`test/linear_wave_tests.jl` 11 -> 19, covering a ragged q0, a ragged p0, both
through the N-and-two-argument forms, and a well-formed ensemble in each branch
as the positive control. `linear_wave_tests.jl` needed `HODEEnsemble`/
`LODEEnsemble` added to its imports.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 30, 2026 13:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Base automatically changed from feature/toda-lattice-handwritten-equations to main July 30, 2026 13:41
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.46%. Comparing base (e289c4d) to head (fb24f98).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   75.22%   75.46%   +0.24%     
==========================================
  Files          43       43              
  Lines        2442     2454      +12     
==========================================
+ Hits         1837     1852      +15     
+ Misses        605      602       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michakraus
michakraus merged commit 9bde45f into main Jul 30, 2026
12 of 15 checks passed
@michakraus
michakraus deleted the fix/check-size-every-ensemble-sample branch July 30, 2026 15:24
michakraus added a commit that referenced this pull request Jul 30, 2026
v0.8.0 was tagged at 52c123e, but three PRs landed after that tag while their
entries were written into the `## [0.8.0]` section: #110 (hand-written Toda
vector fields), #111 (NFC normalization, which had its own `## [Unreleased]`
section) and #112 (the ensemble size assertion). So 0.8.0's section described
nine bullets that v0.8.0 does not contain.

Split by which bullets existed at the v0.8.0 tag, the same way 52c123e split
0.7.4 out of 0.8.0:

  ## [0.8.1] — 2026-07-30   Added (3), Changed (3), Documentation (1), Tests (2),
                            Repository hygiene (1), Known follow-ups
  ## [0.8.0] — 2026-07-30   unchanged but for the nine bullets that moved out

Four cross-references follow from the move, each having pointed at a neighbour
that stayed behind in 0.8.0:

  * "the last of the three conversions" becomes "completing the three
    conversions 0.8.0 began with OuterSolarSystem and LinearWave";
  * "gained the same four-argument methods" becomes "gained the four-argument
    methods LinearWave gained in 0.8.0";
  * the lode_wiring entry's "for the same reason" now names the reason, rather
    than pointing at a LinearWave bullet in the previous section;
  * "the same trap the linear wave hit above" becomes "in 0.8.0".

0.8.0's `Changed` preamble says "unlike the two signature repairs below", which
#110 had made wrong by adding a third; moving the Toda entry out makes the count
correct again.

`Known follow-ups` moves with the release, per 52c123e's convention of listing
the open ones once, under the newest section. Both remaining items are still
open.

The compare-link block gains `[0.8.1]`, `[0.8.0]` and `[0.7.4]`, which were
never added, and `[Unreleased]` is repointed from v0.7.3 to v0.8.1.

Version 0.8.1 rather than 0.9.0: no exported signature changed, the removed
`const Omega` was never exported, and the tightened size assertion rejects input
that previously produced wrong answers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants