Skip to content

lambda_ is an input, so nothing resets it - #109

Merged
dfalster merged 4 commits into
masterfrom
fix/lambda-is-an-input
Aug 20, 2026
Merged

lambda_ is an input, so nothing resets it#109
dfalster merged 4 commits into
masterfrom
fix/lambda-is-an-input

Conversation

@dfalster

Copy link
Copy Markdown
Member

setup_clean_leaf() cleared lambda_, so whether a prescribed
Sperry marginal water cost survived depended on which of two
interchangeable-looking calls came next: set_drivers() kept it,
set_traits() lost it, and neither warned. It is the caller's
input -- read only by profit_psi_stem_Sperry, written by no model
code -- and the only input in that function's list.

Both it and lambda_analytical_ now carry their NA default at the
declaration, so a fresh Leaf still reads NA. Derived state and
solved outputs are cleared exactly as before. Closes #96.

`setup_clean_leaf()` cleared `lambda_`, so whether a prescribed
Sperry marginal water cost survived depended on which of two
interchangeable-looking calls came next: `set_drivers()` kept it,
`set_traits()` lost it, and neither warned. It is the caller's
input -- read only by `profit_psi_stem_Sperry`, written by no model
code -- and the only input in that function's list.

Both it and `lambda_analytical_` now carry their NA default at the
declaration, so a fresh Leaf still reads NA. Derived state and
solved outputs are cleared exactly as before. Closes #96.
@dfalster

Copy link
Copy Markdown
Member Author

Which option, and why

The issue offered two, with no strong view. This is option 2 — leave both calls alone and take the field out of setup_clean_leaf() — because the ownership question has a clean answer once you count the reads:

field written by model code read by model code
lambda_ nowhere profit_psi_stem_Sperry, once (leaf_model.hpp:3217)
lambda_analytical_ nowhere nowhere

lambda_ is Sperry's prescribed marginal water cost. Nothing derives it, nothing solves for it, and it is the only entry in setup_clean_leaf()'s list that is not derived state or a solved output. So the asymmetry was not that set_physiology forgot to clear it — it was that setup_clean_leaf was clearing it at all, which is what the issue suspected.

Option 1 (clear it in set_physiology too) would have made the rule consistent at the cost of making a prescribed input unusable: on the multi-layer path you must call set_physiology before every solve, so "any re-driving clears the prescribed cost" means "you can never prescribe one".

The one way this could have gone wrong quietly

double lambda_; had no in-class initialiser — setup_clean_leaf() was the only thing giving it a value, from both constructors. Deleting the line without adding a default would have left a fresh Leaf reading indeterminate memory, and the symptom would have been an occasional plausible-looking Sperry profit rather than a crash. Both fields carry = util::na_value at the declaration now, and the test asserts NA on a fresh object before it asserts anything about survival.

Verification

  • make -C tests/cpp: 567 checks, 0 failures; operating_points.tsv 576/576 bit-identical; primitives.tsv 544/544 bit-identical across all five tiers.
  • make -C tests/cpp bench builds (CI compiles it and make all does not).
  • Full R suite green, gradient_golden.tsv worst relative difference 0.

Bit-identity is expected rather than reassuring here: lambda_ reaches only profit_psi_stem_Sperry, which no golden path calls. The evidence that the change does anything is the new test's two arms, and the evidence that it does nothing else is that the golden files did not move.

Test shape

Both arms are asserted, deliberately. set_drivers kept the value before this change as well, so a test that checks only the set_drivers arm passes on the code it exists to reject. The C++ test covers construct / re-drive / re-trait / solve-retrait-solve, plus that profit_, opt_psi_stem_ and R_d_ are still cleared — the fix removed two lines, and taking any more reopens hazard 8. The R test is the issue's own reproduction, verbatim, because that is the layer it was found in.

Two things noticed and not fixed here

  1. lambda_analytical_ is dead. Nothing in C++ reads it — it is write-only from R, and plant carries generated getter/setter glue for it too (plant/src/RcppR6.cpp:211). It is treated as an input here for consistency, but the real question is whether it should exist. Deleting it is an API break under hazard 7 (plant binds it by name), so it wants its own issue rather than a quiet removal.
  2. .gitignore was missing tests/cpp/test_primitives, the binary Compare the primitives, against a table of their own #107 added. One line, folded in — it is the kind of stray artefact a git add -A commits by accident.

# Conflicts:
#	inst/include/phylloptim/leaf_model.hpp
@dfalster

Copy link
Copy Markdown
Member Author

Correction, and an interaction with #93

#93 merged after this PR was opened. Two things in my first comment are now wrong, and one of them matters.

The table above is wrong in one cell. I wrote that lambda_ is "written by model code: nowhere". That was true of 4d7a070; #93 added optimise_psi_stem_ProfitMax, which ends with

// Reported so the equivalence above is inspectable rather than asserted: this
// is the lambda that makes optimise_psi_stem_Sperry find the same point.
lambda_ = profitmax_A_max_ / profitmax_k_span_;

So lambda_ is now an input to one optimiser and an output of another. Corrected table:

field written by model code read by model code
lambda_ optimise_psi_stem_ProfitMax (#93) profit_psi_stem_Sperry
lambda_analytical_ nowhere nowhere

It does not change the decision. A field that is an input on any path must not be wiped on the caller's behalf, so option 2 still holds — and the dual role is an argument for it, not against: setup_clean_leaf cannot be right for both roles at once. But it is a genuine design smell, and it is now the subject of #114 (raised by @itowers1 on #93). The field comment says so and points there.

What the merge required beyond the mechanical conflict

The conflict itself was mechanical — #93 added six ProfitMax fields to setup_clean_leaf on either side of the two lines this PR removes. Resolved by keeping all six and dropping both.

The substantive part: #93 wrote this issue's asymmetry into a guard, as documentation of a live bug. optimise_psi_stem_Sperry now refuses a non-finite lambda_, and its comment read:

Note the asymmetry this guards: set_traits() clears lambda_ (via setup_clean_leaf) and set_drivers() does not, so whether it survives depends on the order of two calls that look interchangeable.

with the error message asserting lambda_ is "cleared to NA by the constructor and by set_traits". Both are falsified by this PR. Left alone they would have been the third and fourth places in the tree describing behaviour that no longer exists — so both are rewritten. The guard itself stands unchanged and its test still passes: "the caller never set one" is still reachable, since a fresh Leaf still reads NA.

And the guard has a hole #93 could not have known it had, now documented at the site: because ProfitMax writes lambda_, calling ProfitMax and then Sperry silently optimises ProfitMax's derived λ rather than a prescribed one. Finite, plausible, and not the caller's number, so the guard cannot see it. That is #114's territory, not this PR's.

Verification after the merge

  • make -C tests/cpp: 617 checks, 0 failures; operating_points.tsv 576/576 and primitives.tsv 544/544 bit-identical.
  • make -C tests/cpp bench builds.
  • Full R suite green; gradient_golden.tsv worst relative difference 0.
  • Leaf-to-air VPD, and Sperry's ProfitMax #93's own optimise_psi_stem_Sperry refuses an unset lambda test passes unmodified.

One test added: test_profitmax_reports_its_own_lambda asserts ProfitMax leaves a finite λ that is not the caller's, so the ⚠️ I just wrote into the Sperry guard is a checked statement rather than a plausible one — and so #114's resolution has to update it deliberately.

Also worth noting on #113

The dual role suggests its own fix, which I have added there: ProfitMax's λ is a report, and there is already a dead field named lambda_analytical_ sitting next to it. Moving the report there would make the input/output collision unrepresentable instead of documented.

It changes observable behaviour and moves no numbers, so the
fingerprint cannot signal it and NEWS is the only notice a
consumer gets. Filed under the 0.5.0 heading #111 opened.
@dfalster
dfalster merged commit 2f62073 into master Aug 20, 2026
7 checks passed
@dfalster
dfalster deleted the fix/lambda-is-an-input branch August 20, 2026 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set_traits clears lambda_ and set_drivers does not

1 participant