Repository navigation
Conversation
…anl#1298) Where the value RHS is non-finite at a state with a concentration below zero, cvode_rhs integrates f(clamp(y)) and the Newton matrix is J(clamp(y)) (lanl#135). The sensitivity RHS had only lanl#392's retry, which snaps a component inside (-atol_i, 0). That retry lets a run step a hair past zero; there the value RHS's clamp holds the species where it is, it drifts past -atol_i, and the sensitivity RHS refused every state from then on. CVODES cut h until it gave up and the lanl#176 retry returned another run's trajectory: MODEL2001200002's state 2.6e-3 of its peak off the plain run's, every column 0.28 to 1.0 off a central difference. The sensitivity RHS now retries at clamp_state_nonneg's state too, only where f(y) itself is non-finite, so the columns are those of the flow the run integrates. Where f(y) is finite at a negative base the run integrates f(y), and a NaN column still goes to the lanl#384/lanl#386 refusal; so does a slot negative by design (lanl#706's mask). BNGSIM_SENS_CLAMP_NUMERIC_ZERO=0 turns both retries off. Of 1,190 BioModels run with three columns each, the fix changes three: MODEL2001200002 (state 1.2e-2 -> 1.3e-5 off a 1e-3x-tolerance reference) and two by under 2e-6 of a column's peak.
Collaborator
|
A heads-up before we review this one, which is ours this week: since #1354 merged (main c0877fa), it no longer builds against main.
Could you rebase it on main? #1354 also changed what the clamp does:
So the state this retry evaluates the sensitivity RHS at may want the same We'll review it with #1346 and #1370, which change the same clamp, once all three are on current main. |
This branch has not been deployed
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.
Fixes #1298.
The bug
A sensitivity run on MODEL2001200002 returned a different trajectory from its plain run, and columns that were wrong. The only sign was a warning.
The state row is the corpus job (rtol 1e-6, atol 1e-9). The column rows are the test's fixture at rtol 1e-9, atol 1e-12.
Why
Ldecays to about 1e-16 by t = 25 and sits a hair below zero from there. ThereD = d_im*(L/T)^1.36/(s + (L/T)^1.36)*Tis NaN.f(clamp(y)). The analytical Jacobian does the same, so the Newton matrix isJ(clamp(y)).(-atol_i, 0). That retry is what lets the run step a hair past zero: a state withLin the band is accepted.Lis past zero, the value RHS's clamp holds it at the value it reached rather than restoring it (the clamped RHS has no-λ·Treg·Lterm). It drifts past-atol_i(L = -1.39e-12against atol 1e-12).huntil it gave up, and the Four Linux-only test failures, found by #169's first whole-suite run: the GH #176 FD fallback dies at a second crossing, and a steady-state fixture is exactly singular under reference LAPACK #176 retry returned a run with the finite-difference Jacobian instead.BNGSIM_SENS_CLAMP_NUMERIC_ZERO=0, which turns #392's retry off, gives the right answer on main too, because every NaN then rejects the step and keepsL ≥ 0. So this PR doesn't revert #392, which fixed BIOMD0000000833. It gives the run that #392 lets past zero a consistent way to continue.The fix
After #392's retry declines, the sensitivity RHS retries at
clamp_state_nonneg's state, the one the value RHS integrated. It does this only wheref(y)itself is non-finite.That condition is the point. Where
f(y)is finite at a negative base, the run integratesf(y)there, and a column read atclamp(y)would belong to another state. That is #392'spow(x, 4.0)besidelog(x). Such a NaN still goes to the #384/#386 refusal. The new control test covers this case, and a mutation that drops the condition fails it.On #392's reasoning
#392 kept its band narrow for a stated reason: a base more than atol negative, such as
V_membrane = -61in BIOMD0000000374, has no∂/∂n base^n, so its NaN should be refused rather than answered with a confident 0.That reason still holds, and this PR keeps it, in two ways:
clamp_state_nonnegto slots that are concentrations by construction. A rate-rule target, a promoted parameter, a fixed species and a species declared negative are never touched. SoV_membranekeeps its NaN.f(y)condition. The retry fires only where the run is integrating the clamped flow anyway.I've added a note to #392's comment saying so.
Changes
Plus
python/tests/test_sens_rhs_at_the_clamped_state.pyandchangelog.d/1298.fixed.md.Tests (
test_sens_rhs_at_the_clamped_state.py)The fixture is MODEL2001200002 (Mpekris 2017, BioModels, CC0) as a 7-species Antimony string with its unused parameters dropped, so the test needs no corpus file.
_fd_sens.central_difference(sigma_nk)_fd_sens.central_difference(g_reg)f(y)is finite is still refuseddA/dv = -4at t = 4, exact)The columns are judged by the repo's oracle (
python/tests/_fd_sens.py). Its steps are chosen from the measured noise floor, and it refuses kinks rather than comparing them. It judges the species that stay above atol (C, N, Treg). T, Csc and I fall to 1e-34 and L sits below zero, where a plain run's own error exceeds the value and the oracle (correctly) refuses those cells as kinks.Mutation: removing the
f(y)condition makes only the control fail.Corpus A/B
Every BioModel under 1 MB was run as a sensitivity run on three nonzero primary parameters (compartment sizes excluded), at its job's tolerance, with main's behaviour and with this PR's. That's about 1,190 runs; the rest are refused identically on both sides for unrelated reasons.
The final code gives exactly the prototype's results on all three changed models.
Full suite
16,131 passed, 148 skipped. One failure,
test_perf_ab.py::test_a_relative_interpreter_stays_in_its_venv, fails identically on main locally: macOS spells/tmpas/private/tmp. clang-format 17.0.6 and ruff are clean.CI:
Python suite · ubuntu-latest · 1/2failed one timing check.test_codegen_gen_scaling.py::test_large_model_generation_is_not_quadratictook 22.7 s against its 20 s wall-clock budget. This PR doesn't touch code generation, the test it times. Locally the test takes 4.2 s on this branch and 3.9 s on main, and the same shard passed onubuntu-latest-lapackand macos-14. It reads as a slow runner. A rerun of that job should clear it; I don't have the rights to trigger one.Compatibility and performance
BNGSIM_SENS_CLAMP_NUMERIC_ZERO=0still turns both retries off.Not fixed here: out-of-step reads after the refusal
On main, after the sensitivity RHS's NaN, this model's run reads its state at times outside the current step. Examples:
CVodeGetDkyat t = 49 when the step spans [49.0087, 49.0261];CVodeGetSensDky1at t = 2.6e-312, a garbage time.There are 56 such errors in one run, before it dies with
CV_ILL_INPUT. This reproduces on an untouched main build. With this PR the path is no longer reached on this model, but the defect is still there for any run that reaches it. Filed as #1301.Risks and likely objections
f(clamp(y))at these states, so the retry doesn't invent a state. Refusing there doesn't stop the run either: it ends in the Four Linux-only test failures, found by #169's first whole-suite run: the GH #176 FD fallback dies at a second crossing, and a steady-state fixture is exactly singular under reference LAPACK #176 retry returning another trajectory, with a warning.f(y)is finite, nothing changes. A slot negative by design is the mask's to keep (The GH #135 non-finite-RHS retry sets every negative state component to 0, not just the offending concentration. Legitimately negative rate-rule parameters (e.g. a membrane potential) and negative boundary species then follow the wrong... #706).Remaining risk: a model whose value RHS is non-finite at a negative concentration and whose sensitivity was relying on the refusal to stay non-negative. With the fix, such a model follows its plain run instead. None appears in the corpus. The trajectory it would follow is the plain run's, so the sensitivity run would now match it, and #1297 is where that plain-run behaviour belongs.