Repository navigation
feat: read like R, and explain what cannot be translated - #389
Merged
Merged
Conversation
Pin pmcore =0.29.0, which brings pharmsol 0.29.2. ODE dose inputs and their scales now stay on the derivative RHS instead of being declared as separate routes; analytical models keep explicit routes, lag and fa. - `R[j]`/`rateiv[j]` and `B[j]`/`bolus[j]` may be a standalone additive term or one exact product with a scale, in either operand order. - A bolus scale may not depend on state or dose inputs. - A PM_model rebuilt from another PM_model renders its DSL from `arg_list` rather than inheriting a previously rendered string, which could be stale. - Regenerate data/*.rda, which the data-currency test requires after any change to the R6 method bodies.
Model equations now behave like R wherever the DSL can follow, and every rejection names the call and the fix rather than generated DSL text. Expressions - Conditionals work in any position. The DSL accepts one only as a whole right-hand side, so nested conditionals are hoisted into derived helpers. - pmin/pmax, variadic max/min, ifelse, log(x, base), trunc and sign are translated instead of rejected. - round(x, digits) follows R's half-to-even rule, not the DSL intrinsic, which rounds halves away from zero. - estimate_effect_2/estimate_effect_3 are available with a checked signature. The retired six-argument get_e2 spelling gets a migration message that states the internal w = alpha * u * v convention. - Unsupported calls quote the R expression. Every unknown function previously crashed with `subscript out of bounds`, so the message written for it was never reached. - Missing and non-finite values are rejected with a fix. - A dose input inside a conditional is refused with the supported alternative. Diagnostics - compile() validates the rendered source, so an invalid model fails where it is defined instead of at the start of a fit. - Every emitted line records the R statement behind it, so backend diagnostics name that statement. - The fit-time note about generated line numbers is removed: it is no longer true. Requires pharmsol with the derived-value type fix (LAPKB/pharmsol#372): a derived value initialised with a whole number was typed Int while stored in an f64 buffer, so hoisted helpers returned the bit pattern instead of the value. data/*.rda is regenerated, which the data-currency test requires after a change to the R6 method bodies.
Picks up the derived-value type fix (LAPKB/pharmsol#372): a derived value initialised with a whole number was typed Int while stored in the f64 derived buffer, so reading it returned the bit pattern. The hoisted helpers this branch introduces rely on that fix. pharmsol 0.29.3 pins its sibling crates exactly, so pharmsol-dsl and pharmsol-macros move with it. pmcore 0.29.0 requires `pharmsol = "0.29.2"` (a caret), so no pmcore change is needed and pmcore stays at 0.29.0.
Siel
marked this pull request as ready for review
September 15, 2026 13:33
CI pins `roxygen2@8.1.0` in the R-CMD-check workflow and fails when `man`/`NAMESPACE` differ from a fresh `roxygenise()`. The previous commit hand-edited the Rd in the style of the locally installed 7.3.3, so the list indentation and paragraph wrapping did not match what CI generates. Regenerated with 8.1.0: only PM_model.Rd changes, which confirms the rest of the documentation was already produced by that version.
…390) * feat: close the remaining R gaps and make the emitter self-checking Language - Braced conditionals (`if (cond) { a <- 1; b <- 2 } else { ... }`) are accepted in the secondary, equation, output and initial-condition blocks, which is how R users write alternative parameter sets. A branch that assigns a derivative, output or initial condition is refused with the conditional-equation form to write instead, because the DSL accepts only plain assignments inside an `if`. - A `for` loop is refused with an explanation rather than a generic message: loops are not part of the model language. Correctness fixes found by the new tests - `p1[1]` used to be rewritten into the identifier `p11`, so it failed later as an unknown name. Indexing is now limited to what can be indexed - `x[i]` and the dose inputs - and anything else is reported. - `base::exp(x)` and other calls whose head is itself a call raised a base R error ("the condition has length > 1") because `as.character()` on that head returns several elements. Every call head is now extracted by one helper that returns a single name or nothing, and the model gets a diagnostic. - `round(x, -1)` was rejected: a negative literal is a call to `-` in R's parse tree, so the digit check did not see a number. Emitter structure - Generated lines are carried as a value type that keeps each line and the R statement behind it together, so provenance cannot drift. This replaces the parallel `*_src` vectors that five emitters had to keep in step by hand; the renderer now fails loudly if line and label counts ever disagree. - The `fa` block multiplies the bolus term while the derivative is rendered instead of patching the finished text with `gsub()`, which depended on the term appearing exactly once and would have edited the wrong occurrence if that ever changed. Tests - The numeric corpus grew from 24 to 53 cases: half-even ties, negative digits, nested and chained conditionals, literal-branch conditionals, a covariate-driven branch, and a helper-name clash. Each case runs the model and compares with R's own evaluation, because a translation that parses and computes something else passes every structural check. - Every rejection is now asserted to be a structured error that names the fix, so a raw R error cannot reach a user unnoticed again. * feat: name the file and line in a diagnostic when R kept them `keep.source` is TRUE for an interactive session, for `source()` and for a test run, so model blocks defined there carry source references. The statement label now includes them: From your model, output block (model.R:15): y[1] <- undeclared_name * p1 A plain `Rscript` run has `keep.source = FALSE` and no line information exists, so the label stays as it was; the diagnostic is complete either way. The label is a rendering detail only - the generated DSL is unchanged, which the frozen model corpus confirms.
The previous commit added skip_on_covr() to the R6 members test, but the test that actually fails under covr::package_coverage() is the method-bodies check: covr instruments every R6 method body in place (covr:::traverse_R6), so the live generator can never match the uninstrumented bodies frozen in data/*.rda. The member-set check is unaffected because instrumentation preserves method names, and it passed under real covr in CI (FAIL 1, the method-bodies test). The comment now sits with the test it describes. Verified: both tests pass without covr (3 passes); under R_COVR=true the method-bodies test skips while the members test still runs and passes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
+ Coverage 62.73% 64.77% +2.04%
==========================================
Files 51 52 +1
Lines 22410 22964 +554
==========================================
+ Hits 14059 14876 +817
+ Misses 8351 8088 -263
🚀 New features to boost your workflow:
|
mhovd
approved these changes
Sep 16, 2026
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
3 participants
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.
Makes model equations behave like R wherever the DSL can follow, and makes every rejection name the call and the fix.
Built and verified against pharmsol 0.29.3, which carries the derived-value fix this branch depends on (LAPKB/pharmsol#372). No PMcore change was needed: pmcore 0.29.0 requires
pharmsol = "0.29.2"(a caret), socargo update -p pharmsolmoves pharmsol, pharmsol-dsl and pharmsol-macros to 0.29.3 while pmcore stays at 0.29.0.Expressions
2 * if (wt > 70) v1 else v2andif (a > 0) (if (b > 0) 1 else 2) else 3both work.pmin/pmax, variadicmax/min,ifelse,log(x, base),truncandsignare translated instead of rejected.round(x, digits)follows R's half-to-even rule, not the DSL intrinsic, which rounds halves away from zero.round(2.5)is 2 in R and was 3 in the DSL.estimate_effect_2(u, v, alpha, h1, h2)andestimate_effect_3(...)are available with a checked signature, so a wrong arity is reported against the R call.get_e2(a, b, w, h1, h2, alpha_s)gets a migration message that states the internalw = alpha * u * vconvention, so a reader can check their ownw.Two silent failures fixed on the way: every unknown function (including
get_e2) crashed withsubscript out of boundsbecause the lookup that produced the helpful message could not be reached; and missing or non-finite values now fail with a fix instead of reaching the backend.Errors point at the model, not at generated text
compile()validates the rendered source, so a model that cannot compile fails where it is defined rather than at the start of a fit.From your model, output block: y[1] <- undeclared_value * x[1].Verification
round()tie case.testthatsuite against released pharmsol 0.29.3: 0 failures.data/*.rdaregenerated, as the data-currency test requires after a change to the R6 method bodies. The five data-only fixtures that were merely re-serialized were restored so the diff shows only what changed.Note on the branch
Branched from
issue-385-dose-inputs(the PMcore 0.29 upgrade, #388), so the diff may include that change until #388 lands. It will be rebased ontomainafterwards.