From 4c123df9612ad1065dbf37c84b059def5036c0f8 Mon Sep 17 00:00:00 2001 From: Daniel Falster Date: Thu, 20 Aug 2026 12:58:45 +1000 Subject: [PATCH] Retire claims that #55 and #93 outlived Four sites described the model as it was before two merged fixes. The one that mattered is user-facing: fitting.Rmd told readers that changing a trait and re-driving takes a temperature-cache hit and silently reports the first vcmax it ever saw. #55 put every input of the temperature block in the key, so it does not. The advice to use set_traits() stands on the splines, the solved operating point and the #25 checks instead. Comments and prose only; both golden files bit-identical. The closed_form/#93 divergence is recorded, not repaired -- see NEWS. --- NEWS.md | 23 ++++++++++++++++++++ inst/RcppR6_classes.yml | 15 ++++++++++--- inst/include/phylloptim/closed_form.hpp | 13 +++++++++++ tests/cpp/test_leaf.cpp | 29 +++++++++++++++++++------ vignettes/fitting.Rmd | 23 +++++++++++++++----- 5 files changed, 87 insertions(+), 16 deletions(-) diff --git a/NEWS.md b/NEWS.md index e82c5fd..e31844a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,28 @@ # phylloptim 0.5.0 +## Documentation: claims that #55 and #93 outlived + +No behaviour change. Four places still described the model as it was before two +merged fixes, and one of them was user-facing advice: + +- `vignettes/fitting.Rmd` told readers that a bare `l$vcmax_25 <- x` followed by + re-driving takes a temperature-cache **hit** and silently reports numbers from the + first `vcmax` it ever saw. #55 widened the cache key to every input of the + temperature block, so that is no longer true. The advice — use `set_traits()` — is + unchanged; the reasons are now the splines, the solved operating point and the #25 + checks, which no re-driving repairs. +- `inst/RcppR6_classes.yml` carried the same claim, contradicting the correct + statement 150 lines above it in the same file. +- `tests/cpp/test_leaf.cpp` claimed a sub-assertion isolated `set_traits`' cache + invalidation. It cannot any more: with `vcmax_25` in the key the cache misses + regardless, so the trap is closed twice over and the test cannot say which + mechanism carried it. The two halves are covered separately, and it now says so. +- `inst/include/phylloptim/closed_form.hpp` divides by `atm_vpd_` throughout, which + #93 replaced with the leaf-to-air `vpd_leaf_` in the live model. Recorded rather + than repaired: it is dormant, PM-untested code, and the substitution may not be a + rename — `vpd_leaf_` depends on Tleaf, which depends on the E these expressions + solve for. + ## `lambda_` is a caller input, so nothing resets it (#96) `setup_clean_leaf()` cleared `lambda_`, so whether a prescribed Sperry marginal water diff --git a/inst/RcppR6_classes.yml b/inst/RcppR6_classes.yml index d3d5fcb..3974511 100644 --- a/inst/RcppR6_classes.yml +++ b/inst/RcppR6_classes.yml @@ -380,9 +380,18 @@ Leaf: # A METHOD, NOT FIFTEEN SETTABLE FIELDS. The traits are public doubles, so # `access: field` entries would have worked and would have been wrong: a bare # field write bypasses the #25 invariant checks, leaves the two pre-integrated - # vulnerability splines describing the previous curve, and -- worst -- leaves - # vcmax_/jmax_/R_d_ stale, because set_physiology derives them behind a cache - # keyed on (leaf_temp, atm_o2_kpa) alone and so takes the hit on the next call. + # vulnerability splines describing the previous curve, and leaves the solved + # operating point describing the old traits (hazard 8). + # + # This used to carry a third reason -- that vcmax_/jmax_/R_d_ would go stale + # behind a temperature cache keyed on (leaf_temp, atm_o2_kpa) alone, so that + # re-supplying the drivers took a cache HIT and never recomputed. **#55 closed + # that**: the key is every scalar `update_temperature_dependent_params()` + # reads, `vcmax_25` and `jmax_25` among them, exactly as the temperature-response + # block above already says. The two reasons that remain are enough, and this + # comment contradicting that one for two releases is why the claim is stated in + # one place now. + # # The leaf_model.hpp declaration has the full reasoning; it is the same # judgement as supply_kind above, reached from a different direction. # diff --git a/inst/include/phylloptim/closed_form.hpp b/inst/include/phylloptim/closed_form.hpp index f4300c4..a176876 100644 --- a/inst/include/phylloptim/closed_form.hpp +++ b/inst/include/phylloptim/closed_form.hpp @@ -32,6 +32,19 @@ // within_guard below and PLAN.md item 9) before it can replace the exact solve on // a production path. // +// ⚠️ AND IT HAS DRIFTED FROM THE SOLVE IT APPROXIMATES. Every `D` below is +// `atm_vpd_`, the deficit at AIR temperature, because that is what Fick's law +// divided by when this was written. #93 moved the live model to `vpd_leaf_`, the +// leaf-to-air deficit, which on the energy-balance path is 3-4x the air's over +// Tair 25-45 -- so on that path these formulae now approximate a model that no +// longer exists. Off it the two are equal by construction (`set_leaf_vpd` returns +// `atm_vpd_` exactly), so the prescribed-temperature path is unaffected. +// +// Not repaired here: it is a numerics change to dormant, PM-untested code, and the +// substitution is not obviously just a rename -- `vpd_leaf_` depends on Tleaf, +// which depends on E, which is what these expressions solve for, so the closed +// form may not stay closed. Filed rather than guessed. +// // FOUR THINGS NOT TO GET WRONG, all learned the expensive way over there: // // 1. `newton_steps = 1` is deliberate. Two steps are *worse* in the tail. diff --git a/tests/cpp/test_leaf.cpp b/tests/cpp/test_leaf.cpp index d79ffac..70553b0 100644 --- a/tests/cpp/test_leaf.cpp +++ b/tests/cpp/test_leaf.cpp @@ -2770,19 +2770,34 @@ void test_set_traits_matches_a_fresh_leaf() { "transpiration is bit-identical" + what); } - // The specific trap, isolated, because the bit-exact comparisons above would - // also pass if set_traits rebuilt everything unconditionally and the reason it - // works were lost. vcmax_ is derived inside set_physiology's temperature cache, - // which is keyed on (leaf_temp, atm_o2_kpa) and NOT on the traits -- so this is - // the assertion that "change the trait, then set the drivers again" is a - // sufficient recipe, which it is only because set_traits invalidates the cache. + // "Change the trait, then set the drivers again" really does recompute the + // temperature block, checked end to end through the route a caller takes. + // + // ⚠️ THIS NO LONGER ISOLATES set_traits' CACHE INVALIDATION, and the comment + // here claimed it did for two releases. It said vcmax_ is derived behind a cache + // "keyed on (leaf_temp, atm_o2_kpa) and NOT on the traits", so the recipe worked + // "only because set_traits invalidates the cache". #55 widened the key to every + // scalar update_temperature_dependent_params() reads -- `vcmax_25` included -- + // so the trap is now closed TWICE OVER: the key would miss on the changed trait + // even if set_traits cleared nothing. The assertion below cannot tell which + // mechanism carried it, and pretending otherwise is how a test comes to describe + // a guarantee it is not testing. + // + // The two halves are covered separately and deliberately: + // * the KEY -- test_temperature_params_invalidate_cache, which sets a response + // parameter and re-drives without going through set_traits at all; + // * set_traits' CLEARING -- the bit-exact comparisons above (the splines and + // the solved operating point are not in any key), plus the NA assertions in + // test_prescribed_lambda_survives_redriving. { phylloptim::Leaf l = make_leaf(d, {2.0}, {1.0}); const double vcmax_before = l.vcmax_; l.set_traits(96.0 * 2.0, 2.680147, 3.898245, 5.870283, 2.680147, 3.898245, 5.870283, 1.5, 157.44, 0.30, 0.7, 0.99, 7.5, kRd25); std::vector mrp{1.0 / d.area_leaf}, psi_soil{2.0}, depth{1.0}; - // The SAME leaf_temp and atm_o2_kpa, which is what arms the cache. + // The same leaf_temp and atm_o2_kpa -- which used to be the whole key, and so + // used to be what ARMED the trap. It no longer is: `vcmax_25` has moved, so the + // key differs whatever set_traits did. l.set_physiology(fixture::root_network(mrp, depth), d.PPFD, psi_soil, depth, d.K_s * d.theta / d.h, d.atm_vpd, d.ca, d.leaf_temp, d.atm_o2_kpa, d.atm_kpa); near(l.vcmax_ / vcmax_before, 2.0, 1e-12, diff --git a/vignettes/fitting.Rmd b/vignettes/fitting.Rmd index e4f063a..25ea354 100644 --- a/vignettes/fitting.Rmd +++ b/vignettes/fitting.Rmd @@ -80,12 +80,23 @@ on an existing object; constructing a fresh one costs ~204 µs against ~4 µs to re-trait, which is 73 solves thrown away per evaluation. In a fit that dominates everything else. -⚠️ **And do not write `l$vcmax_25 <- x` instead.** It compiles and is wrong: the -derived `vcmax_`, `jmax_` and `R_d_` live in a temperature cache keyed on -`(leaf_temp, atm_o2_kpa)` and *nothing else*, so changing a trait and re-setting -the drivers takes a cache **hit** and never recomputes them. A sweep written that -way runs at the first `vcmax` it ever saw and reports plausible numbers -throughout. `set_traits()` refreshes all of it. +⚠️ **And do not write `l$vcmax_25 <- x` instead.** It compiles and is wrong, though +not for the reason this vignette used to give. Two pieces of derived state describe +the old trait afterwards and no re-driving repairs either: the **pre-integrated +vulnerability splines** (`stem_b`/`stem_c` and `root_b`/`root_c` each own one), and +the **solved operating point**, which no code path rewrites — so it goes on reading +as though it belonged to the new traits. A bare write also bypasses the +positive-magnitude checks on `psi_crit` and the two `b` parameters. + +`set_traits()` clears all of it, and is the only route bound to R. + +*Corrected in 0.5.0:* this warning used to say the derived `vcmax_`, `jmax_` and +`R_d_` sat behind a temperature cache keyed on `(leaf_temp, atm_o2_kpa)` and +*nothing else*, so that re-supplying the drivers took a cache **hit** and silently +kept the old response. That was true and is not: since the cache key became every +input of the temperature block — `vcmax_25` and `jmax_25` among them — re-driving +after a bare trait write *does* recompute those three. The advice is unchanged; the +reason for it is narrower. ```{r} l <- leaf_model(truth)