diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dab4e2a..e33f4d82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,66 @@ in `CMakeLists.txt`) is derived from it. ### Changed +- **`MODEL2002070001`'s `INVALID_REFERENCE` premise is relaxed: a reference that + answers `NaN` is not a comparison basis, whatever bngsim did (#485).** The + 0.15.0 sweep flagged the entry stale — `natural=DIFF, rr_finite=False` — and it + was half right. The entry claimed *both* engines were broken; bngsim's half of + that stopped being true in 0.14.0, when the #353 unit-volume substitution for a + non-finite compartment size let it integrate the model. RoadRunner's half is + unchanged. So the natural verdict resurfaced as a `DIFF` with `max_rel_err` + `inf`, 606/707 cells failing — RoadRunner's `NaN` scored against bngsim as a + divergence, the one thing it is not. + + 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 no reaction, and no `` block in the file + names `mucosa` or `lumen` — nothing reads a size, so the IVP is well posed in + amounts and bngsim's substitution warning describes a value nothing reads. + 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 and agrees + with bngsim cell for cell. The `NaN` reaches RoadRunner's state through the + compartment, not through the dynamics. + + At the sweep tolerance (rtol 1e-9 / atol 1e-12, `t` = 0..100, 101 points): + bngsim 707/707 cells finite; RoadRunner 2.9.2 runs without raising and returns + 101/707; AMICI 1.0.1 confirms bngsim's trajectory independently, 0/707 cells + failing at `max_rel_err` 0.0. Two engines produce *a* trajectory here and the + third produces `NaN`, which is all the override needs to claim: RoadRunner's + output is not a usable comparison basis. + + `_apply_invalid_reference` now keys the premise on the reference alone — RR ran + and emitted non-finite output — instead of also requiring that bngsim failed. + What that requirement was really doing was guarding against burying a bngsim + defect, and a model bngsim can now integrate needs a guard that still works: + `_reference_nonfinite_covers` re-runs the same 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. + + Two shapes the relaxation deliberately does **not** cover. `rr_finite is None` + — RoadRunner raised, or a segfaulted child left no per-engine status — stays + stale on anything but a natural `BAD_TEST`: "the reference raised" and "we + never learned" are indistinguishable there, and `REFERENCE_FAILED` is already + the right bucket for the first. A natural `PASS` stays stale too: if the + compared columns agreed, the reference was usable for the comparison whatever + it did elsewhere. + + The row lands on `BAD_TEST`, not `PASS`. `PASS` is the #482/#483 treatment for + a divergence attributable to the reference engine, and it would score + RoadRunner as having *validated* a model it answered `NaN` on. `BAD_TEST` says + what happened: the model's own SBML put a `NaN` in the reference's state, and + no comparison exists. Both are non-scoring, so neither costs bngsim a model. + + Suite-side only — no bngsim behavior changes. `overrides.py` rewrites the entry + (now citing #485, the AMICI confirmation, and the `x7` control), + `ode_jobs.json` re-bakes the one job record, and `test_outcome_classify.py` + pins the relaxed premise and the new guard. This was the last `DIFF` in the ODE + sweep: 1237 `PASS` / 27 `REFERENCE_FAILED` / 54 `BAD_TEST` / 5 `TIMEOUT` over + 1323 jobs, no `DIFF` and no `EXCEPTION` row, and no override flagged stale. + - **BIOMD0000000627's rr_parity divergence is allow-listed as a `KNOWN_ARTIFACT`: RoadRunner steps over a smooth pre-stimulus ramp, and bngsim is the correct engine (#482).** Reported as a 0.13.0 regression on the premise that the diff --git a/parity_checks/amici_parity/README.md b/parity_checks/amici_parity/README.md index 8feb6290..3490fd9d 100644 --- a/parity_checks/amici_parity/README.md +++ b/parity_checks/amici_parity/README.md @@ -140,9 +140,13 @@ Two dispositions, and the difference is which engine (if either) is at fault: agreed and they did not — this is the mirror of what #319/#323 did for bngsim's own declared refusals (`UNSUPPORTED` rather than a silent `EXCEPTION`): name the gap, do not claim the win. `REFERENCE_FAILED` and **not** `BAD_TEST`, because -`BAD_TEST` means neither engine could run the model, and here bngsim did. -(rr_parity's `invalid_reference` lands on `BAD_TEST` for exactly the reason it -does not apply here: *there*, bngsim had also failed.) `COMPARISON_ARTIFACT` is +`BAD_TEST` means the *model or test* is the problem, and here it is the reference +engine: AMICI returns finite numbers that a defect of its own makes wrong. +(rr_parity's `invalid_reference` lands on `BAD_TEST` because what it covers is +the other case — a reference that answers `NaN` on a model whose own SBML is +what put the `NaN` there. It no longer requires bngsim to have failed too, since +GH #485; the guard against burying a bngsim defect is now that every surviving +failing cell must sit in a column the reference left non-finite.) `COMPARISON_ARTIFACT` is correspondingly **narrower** than rr_parity's `KNOWN_ARTIFACT`, which also covers "the reference engine has the bug" — that case is `INVALID_REFERENCE` here, precisely so a reference defect is never dressed up as agreement. diff --git a/parity_checks/rr_parity/ode_jobs.json b/parity_checks/rr_parity/ode_jobs.json index 9ab05522..7d0d3d5b 100644 --- a/parity_checks/rr_parity/ode_jobs.json +++ b/parity_checks/rr_parity/ode_jobs.json @@ -1359,7 +1359,7 @@ {"model_id":"MODEL2001200002","input_format":"sbml","method":"ode","reference_engine":"roadrunner","model":"models/MODEL2001200002/MODEL2001200002.xml","oracle":{"metric":"max_rel_err","tol":0.0001},"params":{"sbml_origin":"biomodels_dir","sedml":null,"horizon_source":"invented","initial_time":0.0,"t_start":0.0,"t_end":100.0,"n_points":101,"rtol":1e-06,"atol":1e-09},"overrides":[{"field":"no_oracle_adjudicated","value":{"issue":"GH #117","verdict":"confirm"},"reason":"RR CVODE CV_CONV_FAILURE + COPASI failed. 7-species reaction net with pow(L/T,1.36) Hill terms; species underflow (1e-33..1e-41) makes a stiff solver's trial step overshoot negative -> pow(neg,1.36)=NaN. With species floored at 1e-100 in the RHS (60 orders below the smallest physical value), all three independent solvers (Radau, BDF, LSODA) agree with bngsim to max_rel=0 (peak-rel ~2e-7). bngsim confirmed."}],"notes":"invented horizon (placeholder)"}, {"model_id":"MODEL2001310001","input_format":"sbml","method":"ode","reference_engine":"roadrunner","model":"models/MODEL2001310001/MODEL2001310001.xml","oracle":{"metric":"max_rel_err","tol":0.0001},"params":{"sbml_origin":"biomodels_dir","sedml":null,"horizon_source":"invented","initial_time":0.0,"t_start":0.0,"t_end":100.0,"n_points":101,"rtol":1e-06,"atol":1e-09},"overrides":[],"notes":"invented horizon (placeholder)"}, {"model_id":"MODEL2002030001","input_format":"sbml","method":"ode","reference_engine":"roadrunner","model":"models/MODEL2002030001/MODEL2002030001.xml","oracle":{"metric":"max_rel_err","tol":0.0001},"params":{"sbml_origin":"biomodels_dir","sedml":null,"horizon_source":"invented","initial_time":0.0,"t_start":0.0,"t_end":100.0,"n_points":101,"rtol":1e-06,"atol":1e-09},"overrides":[],"notes":"invented horizon (placeholder)"}, -{"model_id":"MODEL2002070001","input_format":"sbml","method":"ode","reference_engine":"roadrunner","model":"models/MODEL2002070001/MODEL2002070001.xml","oracle":{"metric":"max_rel_err","tol":0.0001},"params":{"sbml_origin":"biomodels_dir","sedml":null,"horizon_source":"invented","initial_time":0.0,"t_start":0.0,"t_end":100.0,"n_points":101,"rtol":1e-06,"atol":1e-09},"overrides":[{"field":"invalid_reference","value":{"issue":null},"reason":"No valid reference: RoadRunner runs without raising but its trajectory is non-finite over the horizon (101/707 = 14.3% finite cells, verified 2026-06-03), so it is not a usable oracle. bngsim also fails to integrate this IVP \u2014 CVODE flag=-4 (error test failed repeatedly / |h|=hmin) at t~=0.0134. Neither engine produces a usable result, so this is a BAD_TEST (both broken), not the EXCEPTION the raised-vs-not taxonomy would record (which would mislabel a non-finite RR run as an actionable bngsim bug). If RR ever returns a finite trajectory here the override goes stale and the natural verdict \u2014 including any real bngsim bug \u2014 resurfaces."}],"notes":"invented horizon (placeholder)"}, +{"model_id":"MODEL2002070001","input_format":"sbml","method":"ode","reference_engine":"roadrunner","model":"models/MODEL2002070001/MODEL2002070001.xml","oracle":{"metric":"max_rel_err","tol":0.0001},"params":{"sbml_origin":"biomodels_dir","sedml":null,"horizon_source":"invented","initial_time":0.0,"t_start":0.0,"t_end":100.0,"n_points":101,"rtol":1e-06,"atol":1e-09},"overrides":[{"field":"invalid_reference","value":{"issue":485},"reason":"No valid reference: RoadRunner runs without raising but its trajectory is non-finite over the horizon (101/707 = 14.3% finite cells, re-verified 2026-08-25 against RR 2.9.2), so it is not a usable oracle. Both compartments declare size=\"NaN\"; every species is hasOnlySubstanceUnits=true and no block in the file names either compartment, so the IVP is well posed in amounts and the size is inert \u2014 but RR carries species as concentrations, so x1..x6 (the species in those compartments) are NaN from t=0 while x7, the one rate-rule variable that is not a species, stays finite and agrees with bngsim cell for cell. The NaN reaches RR's state through the compartment, not through the dynamics. bngsim used to fail here too (CVODE flag=-4, error test failed repeatedly / |h|=hmin, at t~=0.0134); since the #353 unit-volume substitution for a non-finite size it integrates the model (707/707 finite) and AMICI 1.0.1 confirms that trajectory independently \u2014 0/707 cells failing, max_rel_err 0.0 at the sweep tolerance. So the half of this entry's original premise that read \"bngsim also failed\" is gone, and the half that matters is not: there is still no reference trajectory to compare against, which is BAD_TEST (no parity signal) rather than the DIFF the taxonomy would record \u2014 a DIFF would score RR's NaN as a bngsim divergence. NOT reclassified to PASS, which would credit RR with validating a model it answered NaN on. If RR ever returns a finite trajectory here, or any surviving failure lands in a column RR kept finite, the override goes stale and the natural verdict \u2014 including any real bngsim bug \u2014 resurfaces."}],"notes":"invented horizon (placeholder)"}, {"model_id":"MODEL2003030002","input_format":"sbml","method":"ode","reference_engine":"roadrunner","model":"models/MODEL2003030002/MODEL2003030002.xml","oracle":{"metric":"max_rel_err","tol":0.0001},"params":{"sbml_origin":"biomodels_dir","sedml":null,"horizon_source":"invented","initial_time":0.0,"t_start":0.0,"t_end":100.0,"n_points":101,"rtol":1e-06,"atol":1e-09},"overrides":[],"notes":"invented horizon (placeholder)"}, {"model_id":"MODEL2003030003","input_format":"sbml","method":"ode","reference_engine":"roadrunner","model":"models/MODEL2003030003/MODEL2003030003.xml","oracle":{"metric":"max_rel_err","tol":0.0001},"params":{"sbml_origin":"biomodels_dir","sedml":null,"horizon_source":"invented","initial_time":0.0,"t_start":0.0,"t_end":100.0,"n_points":101,"rtol":1e-06,"atol":1e-09},"overrides":[],"notes":"invented horizon (placeholder)"}, {"model_id":"MODEL2003040001","input_format":"sbml","method":"ode","reference_engine":"roadrunner","model":"models/MODEL2003040001/MODEL2003040001.xml","oracle":{"metric":"max_rel_err","tol":0.0001},"params":{"sbml_origin":"biomodels_dir","sedml":null,"horizon_source":"invented","initial_time":0.0,"t_start":0.0,"t_end":100.0,"n_points":101,"rtol":1e-06,"atol":1e-09},"overrides":[],"notes":"invented horizon (placeholder)"}, diff --git a/parity_checks/rr_parity/overrides.py b/parity_checks/rr_parity/overrides.py index 24d8d7dd..30b9207f 100644 --- a/parity_checks/rr_parity/overrides.py +++ b/parity_checks/rr_parity/overrides.py @@ -20,16 +20,21 @@ non-finite output (NaN/Inf) over the horizon, so there is no valid reference to compare against. The auto-derived taxonomy keys only on raised-vs-not, so such a job lands in EXCEPTION - (mislabeling it an actionable bngsim bug) when bngsim also - failed; this narrow human disposition reclassifies it to - ``Outcome.BAD_TEST`` (no parity signal). The runner applies it - only while the premise holds — bngsim failed AND RR's output - is non-finite — and otherwise flags the entry STALE (recovery - stays visible; it can never silently mask a real bngsim bug). - ODE-only in practice. Each entry MUST cite evidence (the RR - finite fraction + the bngsim failure mode). The bar is the - same as KNOWN_ARTIFACT: this is *not* a place to bury a real - bngsim defect — a finite RR reference makes it stale at once. + (mislabeling it an actionable bngsim bug) or, once bngsim can + integrate the model, in DIFF (scoring the NaN against bngsim + as a divergence); this narrow human disposition reclassifies + it to ``Outcome.BAD_TEST`` (no parity signal). The runner + applies it only while the premise holds — RR's output is + non-finite, and on a DIFF that non-finite output accounts for + the whole divergence (#485) — and otherwise flags the entry + STALE (recovery stays visible; it can never silently mask a + real bngsim bug). ODE-only in practice. Each entry MUST cite + evidence (the RR finite fraction, and what settles bngsim's + own trajectory: its failure mode, or an independent oracle). + The bar is the same as KNOWN_ARTIFACT: this is *not* a place + to bury a real bngsim defect — a finite RR reference makes it + stale at once, and so does a failing cell in any column RR + kept finite. NO_ORACLE_ADJUDICATED — a REFERENCE_FAILED row (bngsim ran, RoadRunner refused) whose correctness blind spot was closed by an INDEPENDENT @@ -283,23 +288,39 @@ # --------------------------------------------------------------------------- # # Reference engine ran but produced no usable trajectory (non-finite output) -> # reclassified to BAD_TEST. Keyed "model_id:method". `issue` is optional. -# Applied only while the premise holds (bngsim failed AND RR non-finite); a -# finite RR reference or a now-running bngsim makes the entry STALE. +# Applied only while the premise holds (RR non-finite, and on a DIFF only where +# the non-finite columns account for every surviving failure); a finite RR +# reference, or a failure in a column RR kept finite, makes the entry STALE. # --------------------------------------------------------------------------- # INVALID_REFERENCE: dict[str, dict] = { "MODEL2002070001:ode": { - "issue": None, + "issue": 485, "reason": ( "No valid reference: RoadRunner runs without raising but its " "trajectory is non-finite over the horizon (101/707 = 14.3% finite " - "cells, verified 2026-06-03), so it is not a usable oracle. bngsim " - "also fails to integrate this IVP — CVODE flag=-4 (error test failed " - "repeatedly / |h|=hmin) at t~=0.0134. Neither engine produces a " - "usable result, so this is a BAD_TEST (both broken), not the " - "EXCEPTION the raised-vs-not taxonomy would record (which would " - "mislabel a non-finite RR run as an actionable bngsim bug). If RR " - "ever returns a finite trajectory here the override goes stale and " - "the natural verdict — including any real bngsim bug — resurfaces." + "cells, re-verified 2026-08-25 against RR 2.9.2), so it is not a " + 'usable oracle. Both compartments declare size="NaN"; every ' + "species is hasOnlySubstanceUnits=true and no block in the " + "file names either compartment, so the IVP is well posed in amounts " + "and the size is inert — but RR carries species as concentrations, " + "so x1..x6 (the species in those compartments) are NaN from t=0 " + "while x7, the one rate-rule variable that is not a species, stays " + "finite and agrees with bngsim cell for cell. The NaN reaches RR's " + "state through the compartment, not through the dynamics. bngsim " + "used to fail here too (CVODE flag=-4, error test failed repeatedly " + "/ |h|=hmin, at t~=0.0134); since the #353 unit-volume substitution " + "for a non-finite size it integrates the model (707/707 finite) and " + "AMICI 1.0.1 confirms that trajectory independently — 0/707 cells " + "failing, max_rel_err 0.0 at the sweep tolerance. So the half of " + 'this entry\'s original premise that read "bngsim also failed" is ' + "gone, and the half that matters is not: there is still no reference " + "trajectory to compare against, which is BAD_TEST (no parity signal) " + "rather than the DIFF the taxonomy would record — a DIFF would score " + "RR's NaN as a bngsim divergence. NOT reclassified to PASS, which " + "would credit RR with validating a model it answered NaN on. If RR " + "ever returns a finite trajectory here, or any surviving failure " + "lands in a column RR kept finite, the override goes stale and the " + "natural verdict — including any real bngsim bug — resurfaces." ), }, } diff --git a/parity_checks/rr_parity/rr_run.py b/parity_checks/rr_parity/rr_run.py index ce9ad897..b7bdb50e 100644 --- a/parity_checks/rr_parity/rr_run.py +++ b/parity_checks/rr_parity/rr_run.py @@ -41,7 +41,11 @@ REFERENCE_FAILED and BAD_TEST are auto-derived (never a manual list), so a model *leaving* either bucket — because RoadRunner gained support, or bngsim/the loader was fixed — is a visible win on the next run. EXCEPTION now means *only* -"bngsim raised but the reference succeeded" = a real bngsim defect. +"bngsim raised but the reference succeeded" = a real bngsim defect. The one +authored way into BAD_TEST is an INVALID_REFERENCE override, for the case the +raised-vs-not taxonomy cannot see: a reference that *ran* and answered NaN. It +self-stales the moment that stops being true, so it cannot pin a row there +either (``overrides.py``, ``_apply_invalid_reference``). Each REFERENCE_FAILED row is further sub-classified by WHY RoadRunner refused (``JobResult.reference_refusal``, via ``classify_reference_refusal``), because @@ -147,13 +151,47 @@ def _job_overrides( # --------------------------------------------------------------------------- # # Comparison (the oracle, applied in the worker) # --------------------------------------------------------------------------- # -def _compare_ode(bn, rr) -> tuple[str, float, str, str, float]: - """(status, value, comment, metric, tol) for one deterministic job. +def _reference_nonfinite_covers(a: np.ndarray, b: np.ndarray) -> bool: + """Does the reference's non-finite output account for the WHOLE divergence? + + ``a``/``b`` are the aligned bngsim/reference value blocks of a job that came + back ``diff``. True iff the reference emitted a non-finite cell here AND the + same verdict re-run over only the columns it kept finite *passes* — i.e. + every failure that survived the differ's gates is a cell the reference never + produced a number for, and no bngsim divergence is hiding beside it. (A + reference that is non-finite in every compared column has no finite column + left to disagree in, so it covers by construction.) + + Restricting the columns can only make the re-run stricter: ``scale`` is the + file-wide peak over the retained block, so dropping the non-finite columns + shrinks it, which tightens both the file-scaled absolute term and the + significance gate. The claim it licenses is therefore never a loosening. + + This is what lets an INVALID_REFERENCE override hold on a DIFF (#485). Before + that the override also required bngsim to have failed, which was its guard + against masking a bngsim defect; a model bngsim can now integrate has no such + guard, and this replaces it with the stronger, per-cell one. + """ + finite_col = np.isfinite(b).all(axis=0) + if finite_col.all(): + return False # reference finite throughout — nothing to attribute it to + if not finite_col.any(): + return True # no finite reference column remains to disagree in + return bool(differ.deterministic_verdict(a[:, finite_col], b[:, finite_col])["passed"]) + + +def _compare_ode(bn, rr) -> tuple[str, float, str, str, float, bool]: + """(status, value, comment, metric, tol, ref_nonfinite_covers) for one job. Applies the shared ``_core.differ.deterministic_verdict`` protocol (combined abs+rel per-cell tolerance + fail-fraction budget + hard ceilings) over the common species. The reported metric is ``max_rel_err`` = the post-budget worst remaining relative error (0.0 on PASS), gated at ``REL_TOL``. + + ``ref_nonfinite_covers`` is ``_reference_nonfinite_covers`` on a ``diff`` + (False otherwise, including on the two pre-comparison diffs below — a + mismatched time grid or a disjoint species set is a divergence no reference + NaN explains). Only an INVALID_REFERENCE override reads it. """ bn_t, bn_v, bn_n = bn rr_t, rr_v, rr_n = rr @@ -165,6 +203,7 @@ def _compare_ode(bn, rr) -> tuple[str, float, str, str, float]: f"time grid mismatch (bn n={bn_t.shape}, rr n={rr_t.shape})", metric, tol, + False, ) align = rc.align_common(bn_n, rr_n) if align is None: @@ -174,15 +213,18 @@ def _compare_ode(bn, rr) -> tuple[str, float, str, str, float]: f"disjoint species sets: bn={bn_n[:4]} rr={rr_n[:4]}", metric, tol, + False, ) bn_idx, rr_idx, common = align - v = differ.deterministic_verdict(bn_v[:, bn_idx], rr_v[:, rr_idx]) + a, b = bn_v[:, bn_idx], rr_v[:, rr_idx] + v = differ.deterministic_verdict(a, b) status = "pass" if v["passed"] else "diff" comment = ( f"{len(common)} sp; fail {v['n_fail']}/{v['n_cells']} " f"(hard {v['n_hard_fail']}, soft {v['n_soft_fail']}, forgiven {v['budget_forgiven']})" ) - return status, v["max_rel"], comment, metric, tol + covers = _reference_nonfinite_covers(a, b) if status == "diff" else False + return status, v["max_rel"], comment, metric, tol, covers # --------------------------------------------------------------------------- # @@ -254,29 +296,57 @@ def _classify_failure(bn_exc: str, rr_exc: str) -> tuple[str, str]: return "exception", bn_exc # the reference ran but bngsim raised -> bngsim bug -def _apply_invalid_reference(outcome, rr_finite, reason: str, issue, comment: str): +def _apply_invalid_reference( + outcome, rr_finite, reason: str, issue, comment: str, *, rr_covers_diff: bool = False +): """Resolve an INVALID_REFERENCE override against one job's natural result. - Returns ``(outcome, comment, is_stale)``. The override holds only while its - premise does — bngsim failed (natural EXCEPTION or BAD_TEST) AND the - reference is unusable (RoadRunner raised, so ``rr_finite is None``, or ran - with non-finite output, ``rr_finite is False``). Then the job is reclassified - to ``BAD_TEST`` with the reason recorded. Otherwise the premise is broken — a - finite reference (``rr_finite is True``) or a now-running bngsim (natural - PASS/DIFF/REFERENCE_FAILED) — so the natural outcome is kept and ``is_stale`` - is True, surfacing both the recovery and any real bngsim bug the override - would have masked. ``rr_finite`` is True/False when RR ran, None otherwise. + Returns ``(outcome, comment, is_stale)``. The premise is that the reference + produced no usable trajectory, and it holds in exactly three shapes: + + * natural BAD_TEST — RoadRunner raised and so did bngsim (``rr_finite`` is + None, there being no trajectory to test); + * natural EXCEPTION with ``rr_finite is False`` — RR ran but emitted + non-finite output, and bngsim raised; + * natural DIFF with ``rr_finite is False`` **and** ``rr_covers_diff`` — RR + ran non-finite, bngsim produced a trajectory, and every failure that + survived the differ's gates lies in a column RR left non-finite + (``_reference_nonfinite_covers``). + + Then the job is reclassified to ``BAD_TEST`` with the reason recorded. + + The DIFF shape is #485: the override used to require that bngsim had failed + too, which made it self-stale the moment bngsim gained the model — correct + while "bngsim also failed" was the only thing keeping it from burying a + bngsim defect, but wrong as a premise, because a reference that answers NaN + is not a comparison basis whatever bngsim did. ``rr_covers_diff`` is the + replacement guard, and a per-cell one: a real bngsim divergence beside the + NaN leaves a failing cell in a finite reference column and keeps the DIFF. + + Otherwise the premise is broken — a finite reference (``rr_finite is True``), + a reference whose usability cannot be established (``rr_finite is None`` on + anything but a BAD_TEST, e.g. a segfaulted child), a natural PASS (the + compared columns were usable after all), or a DIFF the reference's NaN does + not account for — so the natural outcome is kept and ``is_stale`` is True, + surfacing both the recovery and any real bngsim bug the override would have + masked. ``rr_finite`` is True/False when RR ran, None otherwise. """ tag = f" ({issue})" if issue else "" - if outcome == Outcome.BAD_TEST or (outcome == Outcome.EXCEPTION and rr_finite is False): + holds = outcome == Outcome.BAD_TEST or ( + rr_finite is False + and (outcome == Outcome.EXCEPTION or (outcome == Outcome.DIFF and rr_covers_diff)) + ) + if holds: if outcome == Outcome.BAD_TEST: # already both-broken; just append context extra = f" | {comment}" if comment else "" - else: # record the natural verdict so "bngsim also failed" stays visible + else: # record the natural verdict so what bngsim did stays visible extra = f" | was {outcome}" + (f": {comment}" if comment else "") return Outcome.BAD_TEST, f"invalid reference{tag}: {reason}{extra}", False + covers = f", rr_covers_diff={rr_covers_diff}" if outcome == Outcome.DIFF else "" note = (f"{comment} | " if comment else "") + ( - f"STALE invalid-reference entry{tag}: natural={outcome}, rr_finite={rr_finite}" - " — reference usable / bngsim runs now, re-triage or prune" + f"STALE invalid-reference entry{tag}: natural={outcome}, rr_finite={rr_finite}{covers}" + " — usable reference / a divergence its non-finite output does not" + " account for, re-triage or prune" ) return outcome, note, True @@ -400,9 +470,13 @@ def _worker(spec: dict, q) -> None: # --- both ran: compare (shared _core.differ protocol; metric realized here) --- try: - status, value, comment, metric, m_tol = _compare_ode(bn, rr) + status, value, comment, metric, m_tol, covers = _compare_ode(bn, rr) res["status"], res["value"], res["comment"] = status, value, comment res["metric"], res["tol"] = metric, m_tol + # Only an INVALID_REFERENCE override reads this: on a DIFF, is the whole + # divergence in columns the reference left non-finite? (See + # _reference_nonfinite_covers and _apply_invalid_reference.) + res["rr_covers_diff"] = covers except Exception as exc: res["status"] = "exception" res["exception"] = f"compare: {type(exc).__name__}: {exc}"[:400] @@ -694,18 +768,24 @@ def main() -> int: f"STALE known-artifact entry{tag}: passes on its own now — prune it" ) n_stale_artifact += 1 - # Invalid-reference disposition: the reference engine ran but produced no - # usable trajectory, so a job that landed in EXCEPTION/BAD_TEST is really - # "both broken" -> BAD_TEST with the reason in the comment. Applied ONLY - # while the premise holds (bngsim failed AND no usable reference — RR - # raised, or ran with non-finite output); a finite reference or a - # now-running bngsim flags the entry STALE so recovery (and any real - # bngsim bug the override was masking) resurfaces — never silent. + # Invalid-reference disposition: the reference engine produced no usable + # trajectory, so there is no comparison to score -> BAD_TEST with the + # reason in the comment. Applied ONLY while that premise holds — RR + # raised while bngsim did too, or RR ran with non-finite output (and on a + # DIFF, only where that non-finite output accounts for the whole + # divergence, #485). A finite reference, an unestablishable one, or a + # divergence the NaN does not cover flags the entry STALE so recovery + # (and any real bngsim bug the override was masking) resurfaces. invalid_ref = invalid_refs.get(r.get("key")) if invalid_ref: reason, issue = invalid_ref outcome, comment, stale = _apply_invalid_reference( - outcome, r.get("rr_finite"), reason, issue, comment + outcome, + r.get("rr_finite"), + reason, + issue, + comment, + rr_covers_diff=bool(r.get("rr_covers_diff")), ) n_stale_invalid_ref += int(stale) n_invalid_ref += int(not stale) diff --git a/parity_checks/tests/test_outcome_classify.py b/parity_checks/tests/test_outcome_classify.py index af1871bc..66fad66b 100644 --- a/parity_checks/tests/test_outcome_classify.py +++ b/parity_checks/tests/test_outcome_classify.py @@ -14,11 +14,17 @@ the unattributable `dead` — to the intended `_core.Outcome`; * CLEAN / FAILING partition the whole taxonomy, so a future Outcome added to ALL must be deliberately classified as scoring or not (this test breaks - until it is), and the new buckets land on the non-scoring side. + until it is), and the new buckets land on the non-scoring side; + * `_apply_invalid_reference` — the one authored way into BAD_TEST — holds only + while the reference is genuinely unusable, and `_reference_nonfinite_covers`, + the per-cell guard the relaxed #485 premise rests on: a DIFF is reclassified + only when every surviving failure lies in a column the reference left + non-finite, so a real bngsim divergence beside the NaN cannot be buried. """ from __future__ import annotations +import numpy as np import rr_run from _core import ALL, CLEAN, FAILING, Outcome @@ -160,19 +166,162 @@ def test_invalid_reference_goes_stale_when_reference_becomes_finite(): assert "STALE invalid-reference" in comment -def test_invalid_reference_goes_stale_when_bngsim_runs_again(): - # bngsim now produces a trajectory (natural REFERENCE_FAILED / PASS): the - # "both broken" premise is gone, so keep the natural outcome and flag stale. +def test_invalid_reference_goes_stale_when_reference_is_finite_whatever_bngsim_did(): + # A usable reference breaks the premise outright — the entry is stale on + # every natural outcome, not only the ones where bngsim failed. for natural in (Outcome.REFERENCE_FAILED, Outcome.PASS, Outcome.DIFF): outcome, comment, stale = rr_run._apply_invalid_reference(natural, True, "r", None, "") assert outcome is natural and stale is True assert "STALE invalid-reference" in comment +def test_invalid_reference_reclassifies_a_diff_the_reference_nan_accounts_for(): + # #485: bngsim integrates the model now, RR still answers NaN, so the natural + # verdict is a DIFF scoring RR's NaN against bngsim. There is no comparison + # basis here whatever bngsim did -> BAD_TEST, with the DIFF kept in-line. + outcome, comment, stale = rr_run._apply_invalid_reference( + Outcome.DIFF, False, "RR all-NaN here", None, "fail 606/707", rr_covers_diff=True + ) + assert outcome is Outcome.BAD_TEST and stale is False + assert "invalid reference" in comment and "RR all-NaN here" in comment + assert "was DIFF: fail 606/707" in comment + + +def test_invalid_reference_keeps_a_diff_the_reference_nan_does_not_account_for(): + # THE bug-masking guard for the relaxed premise: RR is non-finite somewhere, + # but a failing cell survives in a column it kept finite — that is a real + # bngsim divergence beside the NaN, and it must stay a DIFF. + outcome, comment, stale = rr_run._apply_invalid_reference( + Outcome.DIFF, False, "r", None, "", rr_covers_diff=False + ) + assert outcome is Outcome.DIFF and stale is True + assert "STALE invalid-reference" in comment and "rr_covers_diff=False" in comment + + +def test_invalid_reference_does_not_swallow_a_pass(): + # RR emitted a non-finite cell somewhere, but the compared columns agreed -- + # the reference WAS usable for the comparison, so the pass stands. + outcome, comment, stale = rr_run._apply_invalid_reference(Outcome.PASS, False, "r", None, "") + assert outcome is Outcome.PASS and stale is True + assert "STALE invalid-reference" in comment + + def test_invalid_reference_does_not_fabricate_bad_test_from_a_dead_worker(): # A segfaulted child is EXCEPTION with no rr_finite recorded (None). We can't # confirm the premise, so the override must not force BAD_TEST — flag stale. + # (The relaxed #485 premise keys on rr_finite is *False*, never on None, for + # exactly this reason: "RR raised" and "we never learned" look alike here.) outcome, comment, stale = rr_run._apply_invalid_reference( Outcome.EXCEPTION, None, "r", None, "" ) assert outcome is Outcome.EXCEPTION and stale is True + assert "STALE invalid-reference" in comment + + +def test_invalid_reference_does_not_reclassify_a_reference_failed_row(): + # RR raised while bngsim ran: that is REFERENCE_FAILED (no oracle), already + # non-scoring and already the right bucket. The override adds nothing and + # must not move it to BAD_TEST, which would read as "bngsim failed too". + outcome, comment, stale = rr_run._apply_invalid_reference( + Outcome.REFERENCE_FAILED, None, "r", None, "" + ) + assert outcome is Outcome.REFERENCE_FAILED and stale is True + + +# --------------------------------------------------------------------------- # +# _reference_nonfinite_covers — the per-cell guard the relaxed premise rests on +# --------------------------------------------------------------------------- # +def test_reference_nonfinite_covers_when_every_bad_column_is_the_references(): + # Two columns; the reference is NaN in the first and agrees in the second -> + # the whole divergence is the reference's, nothing of bngsim's hides in it. + a = np.array([[1.0, 2.0], [3.0, 4.0]]) + b = np.array([[np.nan, 2.0], [np.nan, 4.0]]) + assert rr_run._reference_nonfinite_covers(a, b) is True + + +def test_reference_nonfinite_does_not_cover_a_divergence_beside_it(): + # Same NaN column, but the second column now diverges by 10x — a real + # divergence the reference's NaN cannot account for. + a = np.array([[1.0, 2.0], [3.0, 40.0]]) + b = np.array([[np.nan, 2.0], [np.nan, 4.0]]) + assert rr_run._reference_nonfinite_covers(a, b) is False + + +def test_reference_nonfinite_covers_an_all_nonfinite_reference(): + # No finite reference column remains to disagree in. + a = np.array([[1.0], [3.0]]) + b = np.array([[np.nan], [np.nan]]) + assert rr_run._reference_nonfinite_covers(a, b) is True + + +def test_reference_nonfinite_covers_nothing_when_the_reference_is_finite(): + # A finite reference has no non-finite output to attribute anything to, so + # it can never cover a divergence — however large that divergence is. + a = np.array([[1.0], [30.0]]) + b = np.array([[1.0], [3.0]]) + assert rr_run._reference_nonfinite_covers(a, b) is False + + +def test_reference_nonfinite_covers_ignores_a_column_nan_on_both_engines(): + # Both engines NaN in a column is a zero-diff pass for the differ, and the + # column is dropped from the finite-only re-run either way; a divergence in + # the remaining column still keeps the entry honest. + a = np.array([[np.nan, 1.0], [np.nan, 30.0]]) + b = np.array([[np.nan, 1.0], [np.nan, 3.0]]) + assert rr_run._reference_nonfinite_covers(a, b) is False + + +# --------------------------------------------------------------------------- # +# _compare_ode — the flag reaches the override through the worker's result dict +# --------------------------------------------------------------------------- # +def _runs(bn_v, rr_v, names=("x1", "x7")): + """Two aligned runs on a shared 4-point grid, as _compare_ode takes them.""" + t = np.linspace(0.0, 1.0, 4) + return (t, np.asarray(bn_v), list(names)), (t, np.asarray(rr_v), list(names)) + + +def test_compare_ode_reports_a_diff_the_reference_nan_covers(): + # MODEL2002070001's shape in miniature: the reference is NaN in the species + # column and agrees on the one variable it kept finite (#485). + bn, rr = _runs( + [[1.0, 2.0], [2.0, 2.0], [4.0, 2.0], [8.0, 2.0]], + [[np.nan, 2.0], [np.nan, 2.0], [np.nan, 2.0], [np.nan, 2.0]], + ) + status, value, _comment, _metric, _tol, covers = rr_run._compare_ode(bn, rr) + assert status == "diff" and value == float("inf") + assert covers is True + + +def test_compare_ode_does_not_flag_a_diff_the_reference_nan_leaves_unexplained(): + # Same NaN column, but the finite column diverges too — the reference's NaN + # does not account for that, so the override must not get its licence. + bn, rr = _runs( + [[1.0, 2.0], [2.0, 2.0], [4.0, 2.0], [8.0, 90.0]], + [[np.nan, 2.0], [np.nan, 2.0], [np.nan, 2.0], [np.nan, 2.0]], + ) + status, _value, _comment, _metric, _tol, covers = rr_run._compare_ode(bn, rr) + assert status == "diff" + assert covers is False + + +def test_compare_ode_flags_nothing_on_a_pass(): + bn, rr = _runs( + [[1.0, 2.0], [2.0, 2.0], [4.0, 2.0], [8.0, 2.0]], + [[1.0, 2.0], [2.0, 2.0], [4.0, 2.0], [8.0, 2.0]], + ) + status, _value, _comment, _metric, _tol, covers = rr_run._compare_ode(bn, rr) + assert status == "pass" + assert covers is False + + +def test_compare_ode_flags_nothing_on_a_disjoint_species_set(): + # A pre-comparison diff: no reference NaN explains a species set that never + # lined up, so the override cannot claim it. + bn, rr = _runs( + [[1.0, 2.0], [2.0, 2.0], [4.0, 2.0], [8.0, 2.0]], + [[1.0, 2.0], [2.0, 2.0], [4.0, 2.0], [8.0, 2.0]], + ) + rr = (rr[0], rr[1], ["y1", "y7"]) + status, _value, comment, _metric, _tol, covers = rr_run._compare_ode(bn, rr) + assert status == "diff" and "disjoint" in comment + assert covers is False