Found by the calibration study, which considered it dangerous enough to spend a pipeline target guarding against.
The trap
vcmax_, jmax_ and R_d_ are derived inside set_physiology's temperature cache, which is keyed on (leaf_temp_, atm_o2_kpa_) and nothing else. So the obvious repair after changing a trait — change it, then set the drivers again — takes a cache hit and never recomputes them.
A trait sweep written that way runs the entire sweep at the first vcmax it ever saw, and every number it reports is plausible.
Why this is not already closed by #42
#42 documents the trap, in ?set_traits and in the developer guide, and set_traits() is the correct route precisely because it ends with setup_clean_leaf(). But nothing prevents the wrong route:
- The traits are public plain doubles, so
l$vcmax_25 <- x compiles and runs.
- After it,
set_physiology() is a cache hit and the object keeps the old derived block.
- There is no error, no warning, and no NA — the values stay finite and in range.
Documentation is the weakest possible guard for a failure whose signature is "plausible numbers". Any external user writing a trait sweep is likely to reach for the field write first, because it is the obvious thing and because R users expect fields to be settable.
Options
- Key the cache on what it actually depends on. It currently keys on the drivers but the cached block also depends on
vcmax_25, jmax_25 and rd_to_vcmax_ratio_. Including those makes the obvious repair work, and makes the cache honest. ⚠️ Costs a wider key comparison on a path that runs per solve — measure it, and note hazard 5 before adding anything to the hot path.
- Make a bare trait write impossible from R — bind the traits read-only, so
l$vcmax_25 <- x errors and points at set_traits(). Cheap, and it closes the R-side route completely. It does nothing for a C++ caller.
- Invalidate on write in C++, which means the traits stop being plain public doubles. The largest change, and it collides with hazard 7 (plant's RcppR6 binds these fields by name).
Option 2 plus a measurement of option 1 is probably the right combination: the R surface is where a sweep gets written, and the C++ callers are plant and this package's own tests, both of which already go through set_traits().
Evidence it is worth guarding
The calibration's own guard asserts that a different parameter vector pushed through a warm cache matches a cold build bit-for-bit — and its author noted that the weaker check (same parameters, warm vs cold) "would pass even if set_traits() did nothing at all". That is the right test shape, and something equivalent belongs in this package's suite rather than in a consumer's pipeline.
test_set_traits_matches_a_fresh_leaf covers the correct route. What is missing is a test that the incorrect route fails loudly.
Found by the calibration study, which considered it dangerous enough to spend a pipeline target guarding against.
The trap
vcmax_,jmax_andR_d_are derived insideset_physiology's temperature cache, which is keyed on(leaf_temp_, atm_o2_kpa_)and nothing else. So the obvious repair after changing a trait — change it, then set the drivers again — takes a cache hit and never recomputes them.A trait sweep written that way runs the entire sweep at the first
vcmaxit ever saw, and every number it reports is plausible.Why this is not already closed by #42
#42 documents the trap, in
?set_traitsand in the developer guide, andset_traits()is the correct route precisely because it ends withsetup_clean_leaf(). But nothing prevents the wrong route:l$vcmax_25 <- xcompiles and runs.set_physiology()is a cache hit and the object keeps the old derived block.Documentation is the weakest possible guard for a failure whose signature is "plausible numbers". Any external user writing a trait sweep is likely to reach for the field write first, because it is the obvious thing and because R users expect fields to be settable.
Options
vcmax_25,jmax_25andrd_to_vcmax_ratio_. Including those makes the obvious repair work, and makes the cache honest.l$vcmax_25 <- xerrors and points atset_traits(). Cheap, and it closes the R-side route completely. It does nothing for a C++ caller.Option 2 plus a measurement of option 1 is probably the right combination: the R surface is where a sweep gets written, and the C++ callers are plant and this package's own tests, both of which already go through
set_traits().Evidence it is worth guarding
The calibration's own guard asserts that a different parameter vector pushed through a warm cache matches a cold build bit-for-bit — and its author noted that the weaker check (same parameters, warm vs cold) "would pass even if
set_traits()did nothing at all". That is the right test shape, and something equivalent belongs in this package's suite rather than in a consumer's pipeline.test_set_traits_matches_a_fresh_leafcovers the correct route. What is missing is a test that the incorrect route fails loudly.