Skip to content

sens: read a row ahead from short of the sample, and fire what crosses there (#1276) - #1356

Open
Montana wants to merge 1 commit into
lanl:mainfrom
Montana:fix/1276-sens-row-short-of-sample
Open

Montana wants to merge 1 commit into
lanl:mainfrom
Montana:fix/1276-sens-row-short-of-sample

Conversation

@Montana

@Montana Montana commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1276.

What was wrong

#1205 (#1182) gave a sensitivity run the read-ahead a plain run makes at an output. At a sample it reads the triggers with the state half a root tolerance ahead, and fires what rises there with its sensitivity jump. That holds only where the run stands exactly on the sample (t_now == t_out[i]).

A sensitivity run also takes a row as reached from up to 1e-12 short of it after a step (reached_output). There it took neither branch: the plain-run branch is sens.n_total == 0 only, and the read-ahead branch needs equality. So where another root stops the run that close before the sample, the row holds the state from before an event that crosses at the sample.

#1276's model: E1: at (q >= 2.5): S = S/2 and E2: at (q >= 2.5 − off): R = 1, with q' = k and a sample at 2.5. On main 9a26f5b:

E2 before the sample by plain run sensitivity run
1e-13 [S] 2.75 at t = 2.5 [S] 5.5 at t = 2.4999999999999
5e-13 2.75 at 2.5 5.5 at 2.4999999999995
2e-12 2.75 at 2.5 2.75 at 2.5

The fix (src/cvode_simulator.cpp, the per-row event block)

#1276's first option:

  • Read ahead from short of the row. The read-ahead branch now also takes a sensitivity run standing short of the row (sens.n_total > 0 && t_now < t_out[i]). CVODE's history ends at t_now, so CVodeGetDky refuses the time ahead, and the state there is a step of f from where the run stands. That is the same as the existing restart case (h_last == 0), and within 1e-12 well inside the tolerances.
  • Fire at the row. What rises fires at t_out[i] with its jump, as A sensitivity run records the row before an event that a plain run records after it (#1133's read-ahead is off with sensitivities) #1182's path already does. The run is restarted there, and the row is taken at t_out[i].
  • Read the columns at the row. After that restart, CVODE's last return time is still the earlier t_now, now before where the run starts. CVodeGetSens refused it, and the row's columns were left at 0. For a row restarted this way the columns are read with CVodeGetSensDky(t_ret). Every other row reads as before.

A row where nothing rises is left as it was, at the time the run stands at.

Tests (test_state_trigger_on_a_sample.py)

The oracles are closed forms: q = k·t, so E1 is at t* = 2.5/k, and S = (3 + t*)/2 + (t − t*) after it.

  • test_a_row_reached_from_short_of_the_sample_holds_the_event[1e-13, 5e-13, 2e-12]: the row is at t = 2.5 exactly, [S] equals the plain run's (3, 4, 5, 2.75, 3.25), and [R] = 1. 2e-12 is the control, outside the 1e-12 a row is taken from; it passed before.
  • test_its_columns_jump_at_the_row[1e-13, 5e-13]: dS/dk = 1.25 and dS/dS0 = 0.5 from the row on, 0 and 1 before it.

On main: the four new short-of-row cases fail, and the 2e-12 control passes. Here: all 35 tests in the file pass.

Mutations, each caught:

mutation caught by
never read ahead short of the row all four new cases
the row's columns read with CVodeGetSens as before both column cases (dS/dk at the row is 0, not 1.25)
the row kept at the time the run stood at all four
the state ahead not stepped all four

Measured

  • fd_sens: the nightly sweep (106 jobs on 53 .net models, sensitivities against central differences), run on main's core and on this one. Both give 106 PASS, with identical verdicts.
  • Full suite on this core: 16,402 passed, 0 failed. Its one more skip than main's comes from the environment: test_build_provenance.py skips a case when BNGSIM_ALLOW_STALE_CORE is set, as it was for this run.

…s there (lanl#1276)

A sensitivity run takes a row as reached from up to 1e-12 short of it
after a step. Stopped that close before the sample by another root, it
took neither the plain run's branch nor lanl#1182's read-ahead (which needs
t_now == t_out), and the row held the state from before an event that
crosses at the sample: [S] 5.5 at t = 2.4999999999999 against the plain
run's 2.75 at 2.5.

The read-ahead now takes that row too: the state ahead is a step of f
from where the run stands (CVODE's history ends there), what rises fires
at the row with its jump, the run is restarted there, and the row is
taken at t_out. Its columns are read with CVodeGetSensDky at the row:
after that restart CVODE's last return time is the earlier t_now, and
CVodeGetSens refused it, leaving the row's columns at 0.
@elyfmiller

Copy link
Copy Markdown
Collaborator

Thanks. We reviewed this independently, Opus 5.5 and Fable 5.1, each on our own build of 14da732 merged onto main c0877fa (clean), on macOS arm64 with KLU on.

The fix is right for what it targets, and we found no correctness defect in it. A row a sensitivity run takes as reached up to 1e-12 short of a sample is now read ahead from where the run stands. What crosses there fires with its jump, and the columns are read at the row after the restart.

Two things to settle before merge:

What we checked comes first, so it needn't be redone.

What we checked

check result
The test file on main's core 4 failed, 31 passed: exactly the four new 1e-13 and 5e-13 cases. The 2e-12 control passes. With this PR: 35 passed.
Mutations, each rebuilt never read ahead short of the row: the 4 fail. Columns read with CVodeGetSens as before: the 2 column cases fail. The row kept at the time the run stood at: the 4 fail, with CVODE CV_ILL_INPUT at t = 3. Restored: 35 passed.
Full python/tests on the merged build 16,392 passed, 166 skipped, 0 failed, with BNGSIM_SKIP_AUDIT=strict. The 99 event and sensitivity test files: 3,572 passed.
The branch is bounded For a run with columns, reached_output(after_step) is t_now ≥ t_out − 1e-12, absolute, and it is the inner loop's only exit; stop conditions are applied in Python after the run. So the Euler step y + (t_ahead − t_now)·f never spans more than 1e-12 plus half a root tolerance. capture_event_sens reads s⁻ at the last return time, within 1e-12 of the row. After the re-init at t_out[i], CVodeGetSens would read at the old t_now, outside CVODES' fuzz (5.5e-14 at t = 2.5), which is why CVodeGetSensDky(t_ret) is needed.
Measured error of the new cases S against the plain run: 2.2e-16 relative. Against the closed form: 4.9e-15. The columns: 7.4e-11 absolute.
With our #1326 (#947), merged both test files: 64 passed. A row reached short with a strict/non-strict pair crossing at the sample is sequenced by #1326 through this read-ahead, and refused in a sensitivity run where the held event reads its twin's write. Where it doesn't, the sensitivity run matches the plain run (dS/dk = 1.25). No conflict.
With #1343, merged they conflict at the per-row sensitivity read. Resolved to #1343's side (an unconditional CVodeGetSensDky(cvode_mem, t_ret, 0, …) plus its refusal), keeping this PR's t_now = t_out[i] and dropping the flag: both test files, 39 of 39
Gates changelog fragment 647 characters and well-formed; ruff and clang-format 17.0.6 clean. The PR's CI finished three minutes before #1354 merged, so these merges are the first test of the combination.

1. With #1353 merged, its [E1 first] case fails

On main + this PR + #1353 (they merge clean in text), the two test files give 1 failed, 66 passed. Each of us reproduced this on our own build. The failure is test_event_batch_order_sensitivity.py::test_a_fixed_fire_and_a_moving_one_in_two_batches_are_refused[E1 first]:

Expected regex: 'move differently.*issues #49, #1277'
Actual message: "Simulation failed: Forward sensitivity: events 'E0' and 'E1' fire at the same instant
t=2.500000 but their crossing times move differently with the requested parameters: the first does
not move and the second does. ... (issue #49)."

The run is still refused, and rightly. But E1's root lies within the instant of E0's clock stop, so it waits for it, and this PR's read-ahead then fires the two as one batch at the row. So #49's same-batch check refuses first, and #1353's cross-batch check never runs.

Whichever of the two merges second will be red until either:

2. A refusal this adds, untested

The same mechanism, with a clock event exactly at the sample: S' = 1, q' = k; E1: at (time >= 2.5): S = S/2; E2: at (q >= 2.5 − 1e-13): R = 1, with sensitivity_params=["k"] and samples at 0, 1, 2, 2.5 and 3.

time >= 2.5 time > 2.5
main row at 2.4999999999999, S = 5.5 and R = 0, recorded before both fires (the #1276 defect), no error row at 2.4999999999999, S = 5.5, R = 0
this PR refused: "events 'E1' and 'E2' fire at the same instant t=2.5 … (issue #49)" row at 2.5, S = 5.5, as the plain run
this PR + #1353 refused (#49) refused (#1353's cross-batch check)
plain run row at 2.5, S = 2.75 row at 2.5, S = 5.5

The refusal is consistent with #49 and #1353. Neither tests whether the two events commute (here they do: E1 writes S and E2 writes R). Still, it's a change from a run main completed, and nothing pins it.

We'd suggest a test for a clock trigger at a row reached short, covering both >= and >, and a line in the body.

Low

  1. Merge order with sens: read a row where the run stands when CVODES returns tout from a step that does not span it (#1301) #1343. As above, taking sens: read a row where the run stands when CVODES returns tout from a step that does not span it (#1301) #1343's form of that line leaves this PR's flag unused. A sentence on the two PRs would keep the second rebase from being a guess.
  2. Tolerances. The new cases' bounds carry no reason: rtol 1e-6 against the closed form, atol 1e-6 on the columns, rtol 1e-12 against the plain run. q and S are linear in t, so the measured errors above would give each its reason, or tighter bounds.
  3. Simplification. The new Euler-step block is the h_last == 0.0 block with t_now in place of t_out[i]. One block, with t_base = short_of_row ? t_now : t_out[i] under if (short_of_row || h_last == 0.0), says it once.
  4. A comment. The branch's outer comment says the row "is read ahead the same way from there", but it's an Euler step of f, not CVODE's interpolant; the inner comment says so.

Not checked: the fd_sens sweep (the body reports 106 PASS on both cores), Linux or Windows on today's main, and perf_ab (one RHS evaluation per sensitivity row reached short of a sample).

This branch has not been deployed

No deployments
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.

A sensitivity run stopped by another root within 1e-12 before a sample still records that row before the event (#1182's read-ahead needs t_now == t_out)

2 participants