diff --git a/NEWS.md b/NEWS.md index dce165207..15c4463e4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -860,6 +860,31 @@ were not previously recorded here: ### Minor changes & bug fixes +* **The scenario gateway baseline is re-blessed, and the gateway diffs the numbers as well as the flags** (#639). `tests/testthat/test_data/scenario_baseline.rds` had been recorded against the tree that became #583 and matched no later build, so `PLANT_RUN_SCENARIOS=1` failed on an unmodified `develop`, and every PR that ran it inherited a failure it had not caused. The cheapest way to establish that was to build a second copy of the base branch and re-run, which two PRs each paid for separately. + + **Bisected, one build per commit, at `max_patch_lifetime = 100` in the gateway's own birth-date coordinate.** Nothing between the bless and #633 moves a scenario — the merged #583 (`133499b8`) and #606 (`39c0c657`) both reproduce the recorded R0 on all eight to seven significant figures — and nothing after #619 does either: `745dd600` and `develop` agree on all eight, so #605, #635 and #637 are inert here, as #635's own NEWS entry claims. Two commits moved it: + + | scenario | baseline | after #633 | after #619 = develop | net | + |---|---|---|---|---| + | S01 | 1.488721 | 0.2725883 (0.18x) | 0.4797763 (1.76x) | **0.32x** | + | S02 | 9.963559e-2 | 2.784370e-3 (0.028x) | 1.153315e-2 (4.14x) | **0.12x** | + | S03 | 5.498561e-12 | 1.745197e-10 (31.7x) | 1.665416e-10 (0.95x) | **30.3x** | + | S04 | 4.745912e-13 | 1.357792e-11 (28.6x) | 1.317815e-11 (0.97x) | **27.8x** | + | S05 | 5.888102e-13 | 5.052547e-12 (8.58x) | 2.470008e-11 (4.89x) | **42.0x** | + | S06 | 3.261466e-13 | 5.082346e-12 (15.6x) | 6.570073e-12 (1.29x) | **20.1x** | + | S07 | 1415.200 | 837.6889 (0.59x) | 810.7050 (0.97x) | **0.57x** | + | S08 | 3.230258e-14 | 2.711131e-13 (8.39x) | 2.025578e-14 (0.075x) | **0.63x** | + + So this is inherited, not a live regression: #619 is a declared science change (`TF24@v8 -> v9`), and #633 is the dependency migration. Both were merged without re-blessing, which is what let them accumulate. + + **Read the sizes with the levels.** S03-S06 and S08 sit between 2e-14 and 2e-10, so a 30x there is 30x of a strategy that is numerically extinct either way and means very little. The moves that mean something are S01 (1.49 -> 0.48, crossing R0 = 1, which is the flag that flipped), S02 (0.12x) and S07 (1415 -> 811). #633's own NEWS entry measured +0.36% on the standard reference run and traced it to one constant (`kg_to_mol_h2o` 55.4939 -> 55.509298, itself 2.8e-4 relative); a 30x on a marginal water-limited scenario from a 2.8e-4 input is the expected shape, because R0 there is an exponentially small survivorship integral, and it is exactly why a single mesic reference run is not a substitute for the gateway. + + **Within #633 the move is phylloptim's, not odelia's.** Rebuilt at `b374ce3b` against phylloptim 0.6.0 but odelia held at 0.2.1 — the `LinkingTo` floor relaxed for the probe, nothing else changed — the scorecard is unchanged on all eight, so odelia 0.2.1 -> 0.3.1 is inert here and the 8x-32x is phylloptim 0.2.0 -> 0.6.0, i.e. the seven-model solver unification (phylloptim #126) reaching every water-limited run. Consistent with odelia's two behavioural changes in that range: `stop_domain` rejection (odelia #56) had no declared domain to act on until #619 added one, and rejecting a non-finite error estimate (#54) only bites on a step that was already diverging. + + **It is not #590**, which is what the size of the move first suggested. #590 is an ancestor of the blessed commit, and the baseline's own metadata records `node_density_in_birth_date = TRUE`: the recorded numbers were already in birth date. Provenance is also why the bless commit itself is not the control — the rds was written from a *dirty* tree at `99e44db6`, whose committed `run_scenarios()` still defaulted to `control()` and had no `node_density_in_birth_date` metadata field at all, so the tree that produced it was the one that landed as `133499b8`. + + `test-scenario-gateway.R` now diffs `offspring_production` at a relative tolerance (`SCENARIO_TOL`, default `1e-3`) alongside `observed` and `persists`, and **any** failure prints the full eight-row table. The two flags were the whole diff, and they are coarse: six of the eight scenarios sit at R0 below 1e-9, where a 30x move crosses nothing, so the table above was reported as the single line `persists S01: TRUE -> FALSE`. + * **An out-of-domain hydraulic lookup now names the spline, the point, the domain and the caller** (#576), via odelia >= 0.2.2 and phylloptim >= 0.2.1, both already required. The message used to be odelia's bare "Extrapolation disabled and evaluation point outside of interpolated domain.", which named none of the four — and since the leaf reads the same transport spline from four places and holds a second spline that is its inverse, that sentence did not distinguish the cases that matter. Localising #576 meant instrumenting the call sites by hand to find out which spline was read and at what value; the answer (the *lower* end of the inverse, where the demanded flux is negative) is what showed the spline had been right to refuse. Error text only; no model behaviour changes. `test-leaf.r` now asserts the spline, which end was missed and the caller, rather than the whole sentence — which is phylloptim's to word. * **TF24f's AD gradient branch mirrors the finite-difference branch — 21% faster, same numbers** (#576). `TF24f_Strategy::solve_leaf`'s two gradient methods were asymmetric. The finite-difference branch has, since #530, run `prepare_collar_solve()` once itself and shared it across its profit evaluations, and taken a zero gradient when that call reports the operating point *forced* by feasibility handling rather than chosen from an interval. The AD branch — the default, hence TF24f-only — went through `evaluate_root_collar_psi()`, which hides that return value, so it re-derived the soil-side caches on each of its three leaf evaluations and asked for a gradient even at a collar potential the feasibility analysis had already rejected. The AD branch now does what the FD branch does. diff --git a/agents.md b/agents.md index 8c49558f2..5df8d987f 100644 --- a/agents.md +++ b/agents.md @@ -524,8 +524,13 @@ default and enabled with `PLANT_RUN_SCENARIOS=1`. It runs the full SCM for every TF24 hydraulic scenario in `inst/scenarios/` and diffs the per-scenario outcomes against a recorded baseline — a *baseline diff*, not an "all pass" assertion, so it catches both regressions and improvements (many scenarios are -expected to fail by design). When a changed outcome is intended, re-bless the -baseline with `make bless-scenarios`. See +expected to fail by design). It diffs two classifications (`observed`, +`persists`) and `offspring_production` at a relative tolerance (`SCENARIO_TOL`, +default `1e-3`), and prints the full per-scenario table on any failure. When a +changed outcome is intended, re-bless the baseline with `make bless-scenarios` +— **in its own commit, with the before/after table in NEWS**. A baseline diff +only carries information while the baseline is current, and one that is not +hands every later PR a failure it did not cause (#639). See [notes/plan-tf24-scenario-framework.md](notes/plan-tf24-scenario-framework.md). CI: [.github/workflows/R-CMD-check.yaml](.github/workflows/R-CMD-check.yaml) and diff --git a/tests/testthat/test-scenario-gateway.R b/tests/testthat/test-scenario-gateway.R index ab7997cf8..931d88955 100644 --- a/tests/testthat/test-scenario-gateway.R +++ b/tests/testthat/test-scenario-gateway.R @@ -25,9 +25,8 @@ test_that("scenario outcomes match the recorded baseline", { cur <- current[order(current$scenario_id), ] expect_equal(cur$scenario_id, b$scenario_id) - ## Compare the two *classifications*, not the magnitudes: offspring production - ## differs across platforms, but which side of a threshold it falls on is - ## stable. + ## Three things are diffed, and the third exists because the first two are + ## coarse. ## ## `observed` alone is not enough. It tests `finite && total > 0`, which every ## scenario has satisfied since the crash fixes landed, so it is pinned at @@ -36,12 +35,38 @@ test_that("scenario outcomes match the recorded baseline", { ## `observed` does not change on a single scenario. A guard that survives ## that is not guarding the thing the gateway now reports, so `persists` -- ## the R0 >= 1 axis #572 made the headline -- is diffed as well. + ## + ## `persists` is coarse in the same way, one threshold down. Six of the eight + ## scenarios sit at R0 below 1e-9, so they can move by orders of magnitude + ## without crossing anything. Two changes did exactly that and neither was + ## re-blessed: the dependency migration (#633) multiplied R0 by 8x to 32x on + ## S03-S06 and cut S02 to 3% of its value, and the storage-pool change (#619) + ## moved five scenarios again on top. The whole table they left behind was + ## reported as the single flag `persists S01: TRUE -> FALSE` (#639). So + ## `offspring_production` is diffed too, at a relative tolerance, and *any* + ## failure prints the whole table -- one flipped flag does not distinguish a + ## rounding change from a 32x one. + ## + ## The tolerance is loose enough for floating-point reordering and tight + ## enough to catch the smallest move we have wanted to see: the 0.36% from the + ## phylloptim 0.6.0 / odelia 0.3.1 migration (#633), which went unblessed. + ## Raise it via SCENARIO_TOL if a platform proves noisier than that; the + ## classifications are the part that is meant to be platform-stable. + tol <- as.numeric(Sys.getenv("SCENARIO_TOL", "1e-3")) + + ## NA-safe inequality: a value appearing or disappearing is a change, two NAs + ## are not. Plain `!=` gives NA there, which then indexes with NA and reports + ## a phantom "NA: NA -> NA" row. + differs <- function(x, y) { + (is.na(x) != is.na(y)) | (!is.na(x) & !is.na(y) & x != y) + } + report <- function(col, label) { if (!col %in% names(b) || !col %in% names(cur)) { return(NULL) } - changed <- b[[col]] != cur[[col]] - if (!any(changed, na.rm = TRUE)) { + changed <- differs(b[[col]], cur[[col]]) + if (!any(changed)) { return(NULL) } sprintf("%s %s", label, @@ -50,11 +75,30 @@ test_that("scenario outcomes match the recorded baseline", { collapse = "; ")) } - msgs <- c(report("observed", "observed"), report("persists", "persists")) + ## Relative move in R0. A baseline of exactly zero has no relative scale, so + ## it is a change iff the current value is non-zero. + from <- b$offspring_production + to <- cur$offspring_production + rel <- ifelse(is.na(from) | is.na(to), NA_real_, + ifelse(from == 0, ifelse(to == 0, 0, Inf), (to - from) / from)) + moved <- differs(from, to) & (is.na(rel) | abs(rel) > tol) + + numeric_report <- if (any(moved)) { + sprintf("offspring_production moved on %d/%d scenario(s) (tol %g)", + sum(moved), length(moved), tol) + } + + msgs <- c(report("observed", "observed"), report("persists", "persists"), + numeric_report) if (length(msgs)) { + tbl <- paste(c(sprintf("%-4s %14s %14s %10s", "id", "baseline", + "current", "rel"), + sprintf("%-4s %14.7g %14.7g %10.3g%s", b$scenario_id, + from, to, rel, ifelse(moved, " *", ""))), + collapse = "\n") fail(paste0("Scenario outcomes changed vs baseline ", "(re-bless via `make bless-scenarios` if intended): ", - paste(msgs, collapse = " | "))) + paste(msgs, collapse = " | "), "\n", tbl)) } else { succeed() } diff --git a/tests/testthat/test_data/scenario_baseline.rds b/tests/testthat/test_data/scenario_baseline.rds index aaad620cd..030f2185d 100644 Binary files a/tests/testthat/test_data/scenario_baseline.rds and b/tests/testthat/test_data/scenario_baseline.rds differ