Export record_given_species_params() so callers can record changes without recalculating - #471
Merged
Merged
Conversation
`species_params<-()` records the values a caller changed in `given_species_params` and then rebuilds the species parameters and recalculates every size-dependent rate. Code that fits a species parameter together with the rate array it determines — an optimiser, say — needs the recording but not the rest, which is at best wasted work and at worst undoes the caller's own adjustment. Writing into the `species_params` slot directly avoids the recalculation but is silently reverted the next time anything triggers one, because the value was never recorded as given. That failure is quiet: the model keeps running and returns plausible numbers. Extract the change detection from `species_params<-()` into `record_given_species_params()` and export it, so that such code can record its changes the same way. Only the entries that actually changed are recorded. Recording an unchanged value would turn a calculated species parameter into a given one and stop it responding to the parameters it is derived from, so the comparison is made entry by entry and a parameter is protected only for the species whose value changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gustavdelius
force-pushed
the
record-given-params
branch
from
July 29, 2026 08:56
3f7fb76 to
874f899
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
species_params<-()does two things: it records the values the caller changed ingiven_species_params, and it then rebuilds the species parameters and recalculates every size-dependent rate.Code that fits a species parameter together with the rate array it determines needs the first but not the second. An optimiser that solves for
ksand writes the matchingmetabitself, or forz_extand the matchingmu_b, gets no benefit from the recalculation — and it can be actively wrong, because the recalculation overwrites the arrays the caller just set.The obvious workaround, writing into the
species_paramsslot directly, avoids the recalculation but loses the recording. The value is then silently reverted the next time anything triggers a recalculation, since it was never marked as given. This failure is quiet: the model keeps running and returns plausible numbers. It bitmizerEcopathin four places —setFeedingLevels()lost itshandks, andmatchCatch()its fittedz_extandD_ext, as soon as any species parameter was assigned afterwards.This PR
Extracts the change detection out of
species_params<-()intorecord_given_species_params(given, value, old_sp)and exports it, so that such code can record its changes the same way. It returns the updatedgiven_species_paramsdata frame and does nothing else — no rebuild, no rate recalculation.species_params<-()now calls the same function, so the matrix/list/S4 column handling added in #465 serves both paths and there is one implementation to maintain rather than two. Behaviour ofspecies_params<-()is unchanged.Only entries that actually changed are recorded, which is the important part: recording an unchanged value would turn a calculated species parameter into a given one and thereby stop it responding to the parameters it is derived from. The comparison is per species per parameter, so a parameter is protected only for the species whose value moved, and a parameter the caller left alone is never pinned.
Alternative considered
Adding a
recalculate = FALSEargument tospecies_params<-(). That is possible — replacement functions do take extra arguments — but it would not be enough on its own: the method rebuilds the whole table fromgiven_species_paramsbefore it callssetParams(), so suppressing onlysetParams()still re-derives the calculated parameters while leaving the rate arrays they feed stale. A public setter that can return an internally inconsistent object also seemed like the wrong thing to offer.Testing
Seven new tests in
test-species_params.Rcover recording, the no-change case, per-species recording,NAtreated as a value, a wholly new column, survival of a subsequent recalculation, and the row-count checks. Full suite: 3362 pass, 0 fail.🤖 Generated with Claude Code