test(rr_parity): MODEL2002070001's invalid-reference premise keys on the reference, not on bngsim (#485) - #486
Merged
Conversation
…the reference, not on bngsim (#485) `INVALID_REFERENCE["MODEL2002070001:ode"]` went stale, and the 0.15.0 sweep said so: `STALE invalid-reference entry: natural=DIFF, rr_finite=False`. Half the premise died in 0.14.0 -- the #353 unit-volume substitution for a non-finite compartment size let bngsim integrate the model -- so the natural verdict resurfaced as a DIFF with max_rel_err=inf, 606/707 cells failing. The other half is untouched: RoadRunner still returns NaN. The entry wants relaxing, not pruning. The model is well posed and the NaN is not in its dynamics. Both compartments declare size="NaN", but every species is hasOnlySubstanceUnits="true", the dynamics are 7 rate rules with 0 reactions, and no <math> block in the file names mucosa or lumen. So nothing reads a size and the IVP is well posed in amounts. RoadRunner carries species as concentrations, so x1..x6 -- the six species in those compartments -- are NaN from t=0, while x7, the one rate-rule variable that is a parameter rather than a species, stays finite. That is the control: the NaN reaches RR's state through the compartment, not through the dynamics. At the sweep tolerance (rtol 1e-9 / atol 1e-12, t=0..100, 101 points), re-verified here against bngsim 0.15.0 / RoadRunner 2.9.2 / AMICI 1.0.1: * bngsim 707/707 cells finite; * RoadRunner runs without raising, 101/707 finite -- the finite column is x7, and it agrees with bngsim cell for cell; * AMICI confirms bngsim's trajectory independently: 0/707 cells failing, max_rel_err 0.0. `_apply_invalid_reference` now keys the premise on the reference alone (RR ran and emitted non-finite output) rather than also requiring bngsim to have failed. Two deliberate departures from the issue's `rr_finite is False or rr_finite is None`: `None` is not admitted, because a segfaulted child records the same absence as an RR raise and the dead-worker guard exists for that; and a natural PASS is not admitted, because a reference non-finite only in a column the comparison never used was usable for the comparison. What the dropped "bngsim failed too" clause was really doing was guarding against burying a bngsim defect, so it is replaced rather than dropped. `_reference_nonfinite_covers` re-runs the same `_core.differ` verdict over just the columns the reference kept finite, and the override holds on a DIFF only if that passes -- a real bngsim divergence beside the NaN leaves a failing cell in a finite reference column and the row stays a DIFF. Restricting the columns can only tighten the re-run (`scale` is the peak over the retained block), so the claim it licenses is never a loosening. BAD_TEST rather than PASS: PASS is the #482/#483 treatment for a divergence attributed to the reference engine, and here it would score RoadRunner as having validated a model it answered NaN on. Both buckets are non-scoring either way. Suite-side only; no bngsim behaviour changes. build_ode_jobs.py cannot run in a checkout without $BIOMODELS_SEDML_DIR (and would churn _meta.generated), so ode_jobs.json is re-baked for the one record through the same Override.to_dict() + json separators write_manifest uses, verified to leave the other 1322 records and _meta byte-identical. The amici_parity README's contrast with this override is updated: it turned on the clause this commit removes. Full ODE sweep on this branch: 1237 PASS / 27 REFERENCE_FAILED / 54 BAD_TEST / 5 TIMEOUT over 1323 jobs -- no DIFF, no EXCEPTION, and no override flagged stale. parity_checks/tests: 494 passed, 17 skipped.
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.
Closes #485.
What the sweep was saying
INVALID_REFERENCE["MODEL2002070001:ode"]is stale, and the 0.15.0 sweep says so:Half the premise ("bngsim failed and the reference is unusable") died in 0.14.0,
when the #353 unit-volume substitution for a non-finite compartment size let bngsim
integrate the model. So the natural verdict resurfaced as a
DIFFwithmax_rel_err = inf, 606/707 cells failing — RoadRunner'sNaNscored against bngsimas a divergence, which is the one thing it is not. The other half is untouched:
RoadRunner still returns non-finite output.
Evidence, re-verified here
The model's
NaNis not in its dynamics. Both compartments declaresize="NaN", butevery species is
hasOnlySubstanceUnits="true", the dynamics are 7 rate rules with 0reactions, and no
<math>block in the file namesmucosaorlumen— so nothingreads a size and the IVP is well posed in amounts.
At the sweep tolerance (rtol 1e-9 / atol 1e-12,
t= 0..100, 101 points), againstbngsim 0.15.0 / RoadRunner 2.9.2 / AMICI 1.0.1:
x1..x6NaN fromt = 0max_rel_err = 0.0RoadRunner's one finite column is
x7, the single rate-rule variable that is aparameter rather than a species in a
NaN-sized compartment — and it agrees withbngsim cell for cell. That is a clean control: the
NaNreaches RoadRunner's statethrough the compartment, not through the dynamics.
The change
_apply_invalid_referencenow keys the premise on the reference alone — RR ran andemitted non-finite output — instead of also requiring that bngsim failed.
Two deliberate departures from the issue's
rr_finite is False or rr_finite is None:Noneis not admitted. A segfaulted child records the same absence as an RRraise, and
test_invalid_reference_does_not_fabricate_bad_test_from_a_dead_workerexists for that;
REFERENCE_FAILEDis already the right bucket for "RR raised whilebngsim ran". Nothing is lost — the both-raised case is still covered by the natural
BAD_TESTclause.PASSis not admitted.rr_finiteis whole-array, so a referencenon-finite only in a column the comparison never used was in fact usable for the
comparison; flipping that to
BAD_TESTwould throw away a real pass.What the dropped "bngsim failed too" clause was really doing was guarding against
burying a bngsim defect, so it is replaced, not dropped.
_reference_nonfinite_coversre-runs the same
_core.differverdict over just the columns the reference kept finite,and the override holds on a
DIFFonly if that passes. A real bngsim divergence besidethe
NaNleaves a failing cell in a finite reference column and the row stays aDIFF,flagged stale. Restricting the columns can only tighten the re-run (
scaleis the peakover the retained block), so the claim it licenses is never a loosening.
BAD_TESTrather thanPASS, as the issue asks:PASSis the #482/#483 treatment fora divergence attributed to the reference engine, and here it would score RoadRunner as
having validated a model it answered
NaNon. Both buckets are non-scoring either way.One note worth flagging
amici_paritymakes the opposite call for a superficially similar shape — itsINVALID_REFERENCElands onREFERENCE_FAILED, on the stated grounds thatBAD_TESTmeans neither engine could run the model. That README sentence cited this override's
"bngsim had also failed" clause as the difference, so it is updated here: what separates
them is whose defect it is. There, AMICI returns finite numbers its own defect makes
wrong. Here, the model's own SBML put the
NaNin the reference's state — the model isthe bad test. Both buckets are non-scoring, so nothing hangs on it numerically, but if
you would rather this row read
REFERENCE_FAILEDfor consistency with that suite, it isa small follow-up (it would want an
invalid_resultrefusal class the way AMICI has one).Verification
5 TIMEOUT over 1323 jobs — no
DIFF, noEXCEPTION, and no override flagged stale.This was the last
DIFFafter test(rr_parity): BIOMD0000000627's divergence is RoadRunner stepping over an ungated ramp (#482) #483.parity_checks/tests: 494 passed, 17 skipped (13 new).build_ode_jobs.pycannot run in acheckout without
$BIOMODELS_SEDML_DIR(and would churn_meta.generated), soode_jobs.jsonis re-baked for the one record through the sameOverride.to_dict()write_manifestuses, verified to leave the other 1322 records and_metabyte-identical.