fix(dde): optExpression and past() durations for delay differential equations by mattfidler · Pull Request #1194 · nlmixr2/rxode2

mattfidler · 2026-08-06T03:07:39Z

Closes #1192
Closes #1193

Two defects in how the delay duration of a past(state, tau) <- expr line is
carried through the model machinery. Together they are why the nlmixr2 DDE
vignette has to pass optExpression = FALSE to every fit.

#1192 -- `rxOptExpr()` could not optimize an expression duration

..rxOptLhs() recursed into the duration with the left-hand-side renderer,
which only knows names and the dosing-property heads (/ and ( work only by
accident -- they are there for d/dt(...)). So past(G, exp(lT)),
past(G, tau*2) and past(G, exp(THETA[1]+ETA[1])) all hit
stop("unsupported lhs in optimize expression"), after a stray print() dumped
the duration into the middle of the progress bar.

Only the whole-model path was affected -- .rxDisguiseCmt() hex-encodes the
whole left-hand side for the chunked path -- so the same model failed at 10
lines and optimized at 50. End to end:

optExpression = TRUE  -> Error : stopped optimizing duplicate expressions
optExpression = FALSE -> runs

The duration is now rendered by .rxOptExpr(), the right-hand-side optimizer.
It parses, and it picks up the same rx_expr_ temporary the matching
delay() calls do -- which is what .rxValidatePast() needs to keep matching
them. A bare name, a number and (x) still render byte-identically.

The chunked path cannot do that (the left-hand side is hidden from the optimizer
and restored byte-exactly), so .rxRealignPastTau() re-points a restored
past() duration at the one its delay() calls ended up with. It is text only
-- the duration is never evaluated, it exists so a history can be matched to its
delay() (src/parseCmtProperties.h) -- and it never guesses: optimizing does
not reorder statements, so the pre-optimization text says which optimized
duration each past() line meant; a single duration is unambiguous without it;
anything else is left for the validator to report.

#1193 -- the duration was never resolved in generated models

rx__pastTau_<state>__ is stored as verbatim source text. All four past()
emitters resolved the history right-hand side through the symengine env but
passed the duration straight through, while every delay() had its duration
inlined. A duration written as an intermediate was therefore emitted into a
generated model that intermediate had been dead-code eliminated from:

d/dt(I)=G*exp(THETA[1])-exp(THETA[1])*delay(G, exp(ETA[1]+THETA[3]))
past(G,T)=1*exp(t*exp(THETA[4]))          # T is a dangling symbol

rxSolve() then rejected rxode2's own generated model with
past(G, T): duration 'T' does not match any delay(G, ...). Reachable from
rxode2 alone via rxode2(mod, calcJac = TRUE) / calcSens=, not only through
nlmixr2.

.rxPastFromEnv() / .rxTauFromEnv() now resolve the duration the way the
history already was, by evaluating a surrogate delay(state, tau) in the env so
the rendering is identical to the augmented d/dt() by construction (this also
covers a constant-folded duration, stored as a plain numeric rather than a
Basic). Used at all four sites, including the exported
.rxPastBaseLinesFromEnv() -- its signature is unchanged, so released reverse
dependencies simply get a corrected string.

No runtime behaviour changed. The duration reaches only sbt (the
normalized-text buffer), never sb/sbDt, so it is absent from the generated C
entirely; _rxDelay() calls _rxPast() for any pre-history time regardless.
With .rxValidatePast() stubbed out, a matched and a mismatched duration give
bit-identical solutions. Fits were never silently wrong -- the generated models
just could not be round-tripped through rxSolve().

Verification

Duration matrix on the whole-model path: exp(lT), lT*2, 2^lT, 2*3,
exp(THETA[1]+ETA[1]) all optimize (all errored before); lT, 12.8, (lT)
unchanged.
Both paths keep the pairing (past(G,rx_expr_0) + delay(G, rx_expr_0);
chunked rx_expr_c2_1), and the optimized model solves identically.
calcJac=TRUE / calcSens= emit past(y,exp(lT)), pass validation, and solve
matching the plain model.
nlmixr2 Example 6 (the vignette model): OBJF 552.7717 with optExpression
both TRUE and FALSE, and rxSolve() now accepts the generated FOCEi inner
model in both cases (it errored in both before), identical D[20].
A genuinely wrong duration is still rejected, with the original message, on
both paths.

test-opt-expr.R 81 -> 112 assertions, test-dde-past.R 26 -> 49, both green
locally, as were test-dde.R, test-dde-sens.R and the adjoint suite (3230
assertions passing before the second commit's rewrite; the individual DDE and
optimizer files re-ran green after it). Leaving the rest to CI.

The second commit is the result of an independent review pass, which found three
gaps in the first: a duration containing = split the line in the wrong place;
the delay scan parsed the reassembled model as R, which is rxode2 syntax and not
guaranteed to be valid R; and a state with several distinct delay durations was
skipped outright. Both scans now match parentheses instead, the way
.rxDisguiseDelayChunks() already does.

Follow-up (not in this PR)

~/src/nlmixr2/vignettes/delays.Rmd no longer needs optExpression = FALSE
once this ships.

🤖 Generated with Claude Code

rxOptExpr() could not optimize a past(state, tau) whose delay duration was an expression: ..rxOptLhs() recursed into the duration with the left-hand-side renderer, which only knows names and the dosing-property heads, so past(G, exp(lT)) hit its stop() (after printing the duration into the middle of the progress bar). Only the whole-model path was affected -- .rxDisguiseCmt() hex-encodes the whole left-hand side for the chunked path -- so the same model failed at 10 lines and optimized at 50, and optExpression=TRUE was unusable for a DDE. Render the duration with .rxOptExpr() instead: it parses, and it picks up the same rx_expr_ temporary the matching delay() calls do, which is what .rxValidatePast() needs to keep matching them. The chunked path cannot do that (the left-hand side is hidden from the optimizer and restored byte-exactly), so .rxRealignPastTau() re-points a restored past() duration at the one its delay() calls ended up with. Text only -- the duration is never evaluated -- and only when the state's delay() calls agree on exactly one duration; otherwise the line is left for the validator. Separately, the duration was stored as verbatim source text and emitted unresolved by all four past() emitters, while every delay() had its duration inlined by symengine. A duration written as an intermediate (T <- exp(lT)) was therefore emitted into a generated model that intermediate had been dead-code eliminated from, so rxode2(..., calcJac=TRUE), calcSens= and the nlmixr2 estimation models produced a past() naming a duration no delay() used any more and rxSolve() rejected them. .rxPastFromEnv()/.rxTauFromEnv() resolve the duration the way the history already was, by evaluating a surrogate delay(state, tau) in the env so the rendering is identical to the augmented d/dt() by construction (this also covers a constant-folded duration, stored as a plain numeric rather than a Basic). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Follow-up to the review of the previous commit. .rxRealignPastTau() decided which past() lines to touch by splitting each line at its first "=" and found the delay() durations by parsing the whole reassembled model as R. Three ways that fell short: - a duration holding an "=" (past(G, h(x)==1)) split in the wrong place, so the line was skipped and left unmatched; - the reassembled text is rxode2 syntax and not guaranteed to be valid R, so a parse failure silently skipped every past() line in the model; - a state whose delay() calls used more than one duration was skipped outright, which a two-delay/two-past() model does legitimately. Delimit the past() left-hand side with the parenthesis matching past(, and scan for delay(state, tau) by matching parentheses the way .rxDisguiseDelayChunks() already does, so neither depends on the text parsing as R. Optimizing never reorders statements, so a state's delay() durations keep their order of first appearance: .rxOptExprChunked() now passes the pre-optimization text in and each past() line follows the delay() it came from. A single duration remains unambiguous without it; anything else is still left for .rxValidatePast(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

- drop the explicit return() on the past() branch of ..rxOptLhs() (the if/else chain is the function's last expression) - call the internal .rxValidatePast()/.rxRealignPastTau()/..rxOptLhs() unqualified in the new tests, as the rest of the suite does, instead of going through rxode2:::

Three defects found by an independent review pass over the branch. R/dde.R -- the stored past() pieces are rxode2 source text, where THETA[1] is a subscript; in the symengine env it is the symbol THETA_1_. Evaluating the text directly therefore failed on every mu-referenced model, so .rxTauFromEnv() fell back to the verbatim duration while the matching delay() had its own inlined -- exactly the #1193 mismatch the branch set out to fix, still reachable from rxode2(mod, calcJac=TRUE) with a THETA[] duration. The same eval resolved the history, and an unresolved history is not a Basic, so .rxDelaySensAugment() also emitted no per-parameter sensitivity pre-history at all. Both now go through .rxSeEvalTxt(), which translates with .rxToSE() first. R/rxOptExpr.R -- .rxRealignPastTau() re-pointed a duration that had matched no delay() BEFORE optimizing, so the chunked path silently accepted a duration the whole-model path rejects (past(G,typo) became past(G,rx_expr_0)). It now rewrites only a duration that did match one, and takes the single-duration fallback only when a single past() line claims it, so two histories cannot be collapsed onto one duration with one silently shadowing the other.

…ld read Second independent review pass. .rxSplitStateTau() required a state name to start with a letter, so a model using a leading-dot state (d/dt(.y.1) parses) had its past() line skipped by .rxRealignPastTau() and left unmatched to its optimized delay(). The "did it match a delay() before optimizing" guard now applies only when the pre-optimization scan actually read that state's delay() durations. The scan reads a line at a time, so a delay() split over lines is not read at all, and the guard would have turned its absence into a refusal to re-point.

…e model Third independent review pass. .rxDelayDurs() reads a line at a time, so a delay() split over lines is not read at all -- and both guards then drew a conclusion the scan had not earned: a past() duration missing from the pre-optimization text was taken as already wrong (it may be the dropped call's), and the durations that were read were matched by position against the optimized ones, which are not the same calls in the same order. Either way a history could be re-pointed at another delay() term's duration, and a genuinely wrong duration could be re-pointed into a valid one on the chunked path. The scan now reports that it could not read a call, and .rxRealignPastTau() leaves every line alone when either text is incomplete -- which is what happened before this pass existed, so the validator says whatever it would have said.

Fourth independent review pass. .rxDelayDurs() matched delay( anywhere on a line, comments included, and the pre-optimization text keeps its comments. A comment naming a delay() the model does not have gave the state a duration no delay() call carries, and .rxRealignPastTau() then refused the rewrite the past() line needed, so the optimized model was rejected by .rxValidatePast(). The scan now takes the code part of a line, tracking string literals so a "#" inside one is not a comment; an unbalanced parenthesis in a comment no longer stops it either.

…duration Fifth review pass. .rxRealignPastTau()'s `orig` defaulted to NULL, and on that branch the "did it match a delay() before optimizing" guard was skipped and the single-duration fallback re-pointed anyway -- so the helper could still turn a duration .rxValidatePast() rejects into one it accepts. Without that text there is nothing to tell a duration that stopped matching from one that never did, and the only caller always has it, so it is now required. Also covered: an operator duration (`a*b`), where the emitted past() line carries deparse1()'s spacing and the augmented d/dt() the rxFromSE() form -- it matches and solves identically, which the comment now states accurately.

…ad code only Sixth review pass, one wrong-solution bug and one wrongly-rejected model. Optimizing can DROP a delay() duration -- 0*delay(G,10) folds to 0 -- and the single-duration fallback then re-pointed past(G,10) at the surviving delay(G,20), so a model that .rxValidatePast() should reject was accepted and solved with one delay() term carrying another's history. A state's durations are now only matched up when the two texts agree on how many it has; nothing else is guessed at, and what does not line up is left for the validator to report. The scan also read inside string literals, so a line as harmless as s="delay(G," marked the model unreadable and stopped the realign for all of it, leaving a model the validator rejects. It now takes the code part of a line: the comment dropped and every string's contents blanked, columns preserved.

Seventh review pass, a regression from the sixth. Masking string literals kept the scan from reading a delay() written inside one, but it also rewrote a duration that legitimately holds a string -- rxode2 takes a string comparison in a model expression, so delay(G, 1+(OCC=="first")) was read as 1+(OCC==" ") and the past() line naming the real duration no longer matched it, leaving a valid model rejected. A call is now LOCATED on the code-only form of a line and READ off the line itself; the top-level comma is found in the code-only form too, so a comma or a parenthesis inside a string no longer splits the call. The two forms agree column for column, which is what .rxCodeOnly() blanking rather than removing is for.

mattfidler · 2026-08-07T09:25:17Z

Follow-up: CodeFactor, and seven rounds of independent review

CodeFactor is green (0 issues): the explicit return() on the new past() branch of
..rxOptLhs() is gone, and the new tests call .rxValidatePast() /
.rxRealignPastTau() / ..rxOptLhs() unqualified, the way the rest of the suite does,
rather than through rxode2:::.

The branch was then reviewed by two outside models (Gemini via Antigravity, GPT via
Copilot), alternating until both came back clean. Every finding below was reproduced
before it was fixed, and each fix has a test. Three of the rounds were about code the
review rounds themselves had introduced.

Wrong answers

A dropped duration was guessed at. Optimizing can remove a delay() --
0*delay(G,10) folds to 0 -- and the single-duration fallback then re-pointed
past(G,10) at the surviving delay(G,20). A model .rxValidatePast() should reject
was accepted and solved with one delay term carrying another's history. A state's
durations are now matched only when both texts agree on how many it has; the fallback
is gone and nothing is guessed at.
THETA[]/ETA[] were never resolved. The stored past() pieces are rxode2 source
text, where THETA[1] is a subscript; in the symengine env it is the symbol THETA_1_,
so evaluating the text directly failed on every mu-referenced model. .rxTauFromEnv()
then fell back to the verbatim duration while the matching delay() had its own
inlined -- the past() duration is not resolved when building estimation models: generated DDE model fails .rxValidatePast() ('duration T does not match any delay(G, ...)') #1193 mismatch, still reachable from
rxode2(mod, calcJac = TRUE) with a THETA[] duration. The same eval resolved the
history, and an unresolved history is not a Basic, so .rxDelaySensAugment() also
emitted no per-parameter sensitivity pre-history at all. Both now go through
.rxSeEvalTxt(), which translates with .rxToSE() first.

Models wrongly accepted

.rxRealignPastTau() re-pointed a duration that had matched no delay() before
optimizing, so the chunked path silently accepted past(G,typo) by rewriting it to the
optimizer's temporary -- while the whole-model path rejects it. It now rewrites only a
duration that did match one. (Two later rounds closed the same hole again via the
optional orig argument and an unreadable pre-optimization text; orig is now
required, and an unreadable text concludes nothing.)
Two histories on one state could both be rewritten to the same duration, leaving one
silently shadowing the other.

Models wrongly rejected

.rxSplitStateTau() required a state name to start with a letter, so a leading-dot
state (d/dt(.y.1) parses) had its past() line skipped.
A delay() written in a # comment gave the state a duration no call carries, and the
realign then refused the rewrite the past() line needed.
A delay() written in a string (s = "delay(G," -- a model that parses) marked the
text unreadable and stopped the realign for all of it.
... and then masking strings corrupted a duration that legitimately holds one
(delay(G, 1+(OCC=="first"))). A call is now located on the code-only form of a
line and read off the line itself, the top-level comma likewise, so a comma or
parenthesis inside a string cannot split it. .rxCodeOnly() blanks rather than removes
precisely so the two forms agree column for column.

Considered, deliberately unchanged

An unreadable delay() (one split over lines) stops the realign for every state, not
just its own. That is the safe direction, and such a model errors exactly as it did
before this pass existed.
deparse1() spaces an operator duration out (past(G,a * b) against delay(G,a*b)).
Matching normalizes that away; a test now solves such a model over its whole history
window and gets the reference answer.
With T = exp(lT) and U = exp(lT), past(y,U) against delay(y,T) is rejected as
written but accepted once differentiated, because the generated model resolves both to
exp(lT). Resolving is the point of past() duration is not resolved when building estimation models: generated DDE model fails .rxValidatePast() ('duration T does not match any delay(G, ...)') #1193, the two durations are the same
expression, and the model solves identically to past(y,T). Making the source-model
check resolve durations too would change a check on a path every reverse dependency
executes, so I have left it alone rather than decide that here.

Tests

test-opt-expr.R and test-dde-past.R are at 311 assertions between them (from 107 on
main), all green, and the full local suite is 0 failures / 0 errors / 73,981 passing.

Generated with Claude Code

mattfidler and others added 10 commits August 5, 2026 21:38

mattfidler merged commit daa766a into main Aug 7, 2026
7 of 8 checks passed

mattfidler deleted the fix/dde-past-tau-optexpr branch August 7, 2026 16:07

Navigation Menu

Uh oh!

fix(dde): optExpression and past() durations for delay differential equations - #1194

fix(dde): optExpression and past() durations for delay differential equations#1194
mattfidler merged 10 commits into
mainfrom
fix/dde-past-tau-optexpr

mattfidler commented Aug 6, 2026

Uh oh!

mattfidler commented Aug 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mattfidler commented Aug 6, 2026

#1192 -- rxOptExpr() could not optimize an expression duration

#1193 -- the duration was never resolved in generated models

Verification

Follow-up (not in this PR)

Uh oh!

mattfidler commented Aug 7, 2026

Follow-up: CodeFactor, and seven rounds of independent review

Wrong answers

Models wrongly accepted

Models wrongly rejected

Considered, deliberately unchanged

Tests

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

#1192 -- `rxOptExpr()` could not optimize an expression duration