fix(indLin): evaluate the indLin() forcing at the current states (#1183) - #1199
Open
mattfidler wants to merge 3 commits into
Open
fix(indLin): evaluate the indLin() forcing at the current states (#1183)#1199mattfidler wants to merge 3 commits into
mattfidler wants to merge 3 commits into
Conversation
`indLin(<state>) <- <expr>` supplies the nonlinear forcing `f` in `dy/dt = f(t,y) + A.y`, but the generated `IndF()` took no state vector and codegen skipped the `__zzStateVar__` population loop for `ode_indLinVec`. The state locals kept their `NA_REAL` declaration, so any forcing referencing a compartment (e.g. Michaelis-Menten elimination) solved to `NA` under `method="indLin"`. `t_IndF` now takes `const double *__zzStateVar__` the way its sibling `t_ME` already does, codegen emits the matching signature and populates the state locals, and `indLin()` passes the interval-start state `yp_` -- the same buffer `meOnly()` hands to `ME`. A forcing that references no state is unchanged. No cache-key bump is needed: `inst/include/rxode2parseStruct.h` and `src/codegen.c` are both inside the content digest that `configure` turns into `rxode2.md5`, verified to change with this diff. Same as commit e352215, which gave `t_LAG` a state argument.
CodeFactor/lintr flagged three issues in the block added for #1183: commented_code_linter on the leading comment, object_name_linter on the `k_depot_central` micro constant, and pipe_consistency_linter on `%>%`. The matExp model now uses the string form the neighbouring rxS() test already uses, which keeps the mini-language micro-constant name out of R's namespace, and the event table is piped with `|>` (the package already depends on R >= 4.1). No behavior change -- test-mexp-nonmem still passes 103/103.
Copilot's re-review of #1199 noted that rewriting the matExp model as a string to satisfy object_name_linter dropped brace-front-end coverage for a state-referencing indLin() forcing (the pre-existing braced case at :66 only has a constant forcing). Adds a braced rxode2({...}) variant of the same Michaelis-Menten forcing. It uses a single compartment, so no k_from_to micro constant is needed and no snake_case name enters R -- which is what tripped the linter in the first place. Solved against the equivalent d/dt() model. test-mexp-nonmem: 105 pass.
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
1 participant
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.
Fixes #1183.
indLin(<state>) <- <expr>supplies the nonlinear forcingfindy/dt = f(t,y) + A.y, but the generatedIndF()took no state vector and codegen skipped the__zzStateVar__population loop forode_indLinVec. The state locals kept theirNA_REALdeclaration, so any forcing referencing a compartment (e.g. Michaelis-Menten elimination) solved toNAundermethod="indLin".Correction to the issue text
The states were not read from uninitialized memory.
printDoubleDeclaration()(src/codegen.h:358) emitsdouble <name> = NA_REAL;, andprnt_vars(print_double, lhs=0, ...)skips nothing. So the behavior was a deterministic NaN forcing, not UB -- confirmed against installed 5.1.7, whererxSolve(mexp, e, method="indLin")$centralcame back allNA. The regression test is therefore a plain "not NA, and matches the ODE" check rather than the repeated-solve UB detector the issue proposed.Changes
inst/include/rxode2parseStruct.h:55--t_IndFgainsconst double *__zzStateVar__, mirroringt_MEon the line above.src/codegen.c:397-- emit the matching signature.src/codegen.c:507-- dropode_indLinVecfrom the guard (keepingode_past, whose__zzStateVar__is NULL by design) so the state locals are actually populated. This is the substantive half; the new argument is inert without it. Also covers the legacy$MATFpath (src/parseIndLin.h), which shares the gate.src/expm.cpp:387-- pass the interval-start stateyp_, the same buffermeOnly()hands toME.Every other
t_IndFreference names the type by its typedef, so nothing else needed an edit.Verification
IndFnow containscentral = __zzStateVar__[__DDT1__]*((double)(_ON[__DDT1__]));ahead of the forcing.d/dt()model: no NAs, max abs error 0.0044 athmax=0.001against a peak of 69.7. Error falls 3.75 -> 0.25 -> 0.02 ashmaxrefines, i.e. still first order inhmax(that is what fix(indLin): restore the inductive-linearization iteration #1185 addresses).devtools::test(): FAIL 0 | PASS 79433 (32 warnings, 4 skips, all pre-existing).configureturns intorxode2.md5, and the digest was verified to move (0ef602f2...unfixed vs546084ed...fixed). Same as commite352215292, which gavet_LAGa state argument and touched no version or md5 seed.IndFnort_IndFis referenced by nlmixr2est, babelmixr2, nonmem2rx or monolix2rx, and neither appears in theinst/include/rxode2ptr.hpointer table, so the append-only rule is not engaged.Scoped to #1183 only. #1184 (
wIndLin) and #1185 (restore the fixed-point iteration) are untouched; #1185 is now unblocked.