feat: Use PMcore 0.26#362
Draft
mhovd wants to merge 12 commits into
Draft
Conversation
Not yet released
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates Pmetrics’ Rust backend to the upcoming PMcore 0.26-style API by switching model handling from “compile Rust → load shared library” to “emit pharmsol DSL → JIT compile at runtime”, eliminating the end-user Rust toolchain requirement.
Changes:
- Replace binary model compilation/loading with pharmsol DSL emission + in-process JIT compilation for simulation and fitting.
- Update nonparametric fit configuration to the new PMcore “composed” API (prior + error models + algorithm + output settings at write time).
- Deprecate/retire build tooling and update R wrappers + documentation to the new
model_sourceinterface.
Reviewed changes
Copilot reviewed 17 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rust/src/simulation.rs | Adapts simulation row generation to new PMcore prediction types. |
| src/rust/src/settings.rs | Replaces old PMcore Settings builder flow with a RunConfig composed of new PMcore pieces. |
| src/rust/src/lib.rs | Updates extendr exports/signatures to accept DSL model source instead of model binaries. |
| src/rust/src/executor.rs | Adds DSL compilation + routes simulation/fit through CompiledRuntimeModel variants. |
| src/rust/Cargo.toml | Pins PMcore to a Git rev and enables DSL/JIT feature; bumps supporting deps. |
| R/PMbuild.R | Deprecates PM_build() and turns it into a no-op user message. |
| R/PM_result.R | Ensures rebuilt results keep DSL source for future simulation. |
| R/PM_model.R | Adds dsl field; updates fit/simulate flows to use DSL; adds outeq validation for error models. |
| R/model_dsl.R | New R→pharmsol DSL emitter used by the model object. |
| R/extendr-wrappers.R | Updates R .Call wrappers to the new Rust function signatures (DSL source). |
| NAMESPACE | Removes exports for build/compile utilities tied to the old binary workflow. |
| man/temporary_path.Rd | Removes documentation for the retired temporary_path() API. |
| man/simulate_one.Rd | Updates docs to reflect model_source DSL-based simulation. |
| man/simulate_all.Rd | Updates docs to reflect model_source DSL-based simulation. |
| man/proportional.Rd | Documents new outeq argument for proportional error model. |
| man/PM_model.Rd | Documents new dsl field and updated meaning of compile(). |
| man/PM_build.Rd | Marks PM_build() as deprecated and documents no-op behavior. |
| man/model_parameters.Rd | Updates docs to reflect model_source input. |
| man/is_cargo_installed.Rd | Removes documentation for the retired is_cargo_installed() API. |
| man/fit.Rd | Updates docs to reflect DSL-based fitting interface. |
| man/dummy_compile.Rd | Removes documentation for the retired dummy_compile() API. |
| man/compile_model.Rd | Removes documentation for the retired compile_model() API. |
| man/additive.Rd | Documents new outeq argument for additive error model. |
| DESCRIPTION | Updates rextendr config version metadata. |
| Cargo.toml | Workspace formatting cleanup. |
| Cargo.lock | Locks new transitive dependencies required by DSL/JIT compilation. |
Files not reviewed (8)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
49
to
52
| /// Helper: get a field as a real (f64), with an optional default if it cannot be coerced. | ||
| fn get_real_or(map: &HashMap<&str, Robj>, key: &str, default: f64) -> AnyResult<f64> { | ||
| Ok(get_field(map, key)?.as_real().unwrap_or(default)) | ||
| } |
Comment on lines
+17
to
+21
| # Naming conventions used by the emitter: | ||
| # * States `x[i]` -> `x{i}` (declared in `states = ...`) | ||
| # * Outputs `Y[i]` / `y[i]` -> `outeq_{i-1}` (declared in `outputs = ...`) | ||
| # * Inputs `b[j]`/`bolus[j]` -> `bolus(input_{j-1}) -> x{k}` (route) | ||
| # `rateiv[j]`/`r[j]`-> `infusion(input_{j-1}) -> x{k}`(route) |
Comment on lines
+63
to
+81
| # Indexing: x[i] -> x{i}. Only literal, positive integer indices are allowed. | ||
| if (op == "[") { | ||
| var <- tolower(as.character(expr[[2]])) | ||
| idx_raw <- expr[[3]] | ||
| if (!(is.numeric(idx_raw) && length(idx_raw) == 1)) { | ||
| cli::cli_abort(c( | ||
| "x" = "Dynamic (non-literal) indices are not supported in the DSL backend.", | ||
| "i" = "Use literal indices such as {.code x[1]}." | ||
| )) | ||
| } | ||
| idx <- as.integer(idx_raw) | ||
| if (var %in% c("b", "bolus", "rateiv", "r")) { | ||
| cli::cli_abort(c( | ||
| "x" = "Bolus/infusion inputs may only be used as standalone additive terms in derivative equations.", | ||
| "i" = "Write, for example, {.code dx[1] = -ke * x[1] + rateiv[1]}, not inside a product." | ||
| )) | ||
| } | ||
| return(sprintf("%s%d", var, idx)) | ||
| } |
Comment on lines
+269
to
+277
| if (is.call(lhs) && as.character(lhs[[1]]) == "[") { | ||
| tgt <- tolower(as.character(lhs[[2]])) | ||
| idx <- as.integer(lhs[[3]]) | ||
| if (tgt != "y") { | ||
| cli::cli_abort("Unexpected indexed assignment to {.code {tgt}[{idx}]} in output block.") | ||
| } | ||
| # Pmetrics data uses 1-based OUTEQ labels, so output `y[i]` maps to the | ||
| # DSL output label `outeq_{i}` (numeric label resolves to index i). | ||
| out_lines <- c(out_lines, sprintf("out(outeq_%d) = %s", idx, expr_to_dsl(rhs))) |
Comment on lines
102
to
108
| export(cli_ask) | ||
| export(cli_df) | ||
| export(click_plot) | ||
| export(compile_model) | ||
| export(cor2cov) | ||
| export(create_pmetrics_project) | ||
| export(downloadR) | ||
| export(dummy_compile) | ||
| export(export_plotly) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 27 changed files in this pull request and generated 4 comments.
Files not reviewed (8)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
Comment on lines
+94
to
+98
| let outeq_1based = get_field(&em, "outeq") | ||
| .ok() | ||
| .and_then(|v| v.as_real()) | ||
| .map(|v| v as usize) | ||
| .unwrap_or(i + 1); |
Comment on lines
+18
to
+21
| # * States `x[i]` -> `x{i}` (declared in `states = ...`) | ||
| # * Outputs `Y[i]` / `y[i]` -> `outeq_{i-1}` (declared in `outputs = ...`) | ||
| # * Inputs `b[j]`/`bolus[j]` -> `bolus(input_{j-1}) -> x{k}` (route) | ||
| # `rateiv[j]`/`r[j]`-> `infusion(input_{j-1}) -> x{k}`(route) |
Comment on lines
+35
to
+37
| # Error models, keyed by output slot. A leading "None" placeholder mirrors the | ||
| # convention used by the parsers (`decode_error_model_rows`), so the error | ||
| # model for output `outeq` occupies index `outeq` (1-based) in the array. |
| knitr | ||
| Config/Needs/website: r-lib/pkgdown, quarto | ||
| Config/rextendr/version: 0.4.2 | ||
| Config/rextendr/version: 0.5.0 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 33 changed files in this pull request and generated 2 comments.
Files not reviewed (8)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
Comment on lines
+92
to
+102
| // The output equation this error model applies to (1-based). Fall back to | ||
| // positional order if the field is absent, preserving old behaviour. | ||
| let outeq_1based = get_field(&em, "outeq") | ||
| .ok() | ||
| .and_then(|v| v.as_real()) | ||
| .map(|v| v as usize) | ||
| .unwrap_or(i + 1); | ||
| if outeq_1based < 1 { | ||
| bail!("error_models[{}].outeq must be 1 or greater", i + 1); | ||
| } | ||
| let outeq = outeq_1based - 1; |
Comment on lines
+17
to
+21
| # Naming conventions used by the emitter: | ||
| # * States `x[i]` -> `x{i}` (declared in `states = ...`) | ||
| # * Outputs `Y[i]` / `y[i]` -> `outeq_{i-1}` (declared in `outputs = ...`) | ||
| # * Inputs `b[j]`/`bolus[j]` -> `bolus(input_{j-1}) -> x{k}` (route) | ||
| # `rateiv[j]`/`r[j]`-> `infusion(input_{j-1}) -> x{k}`(route) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 44 changed files in this pull request and generated 3 comments.
Files not reviewed (8)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
Comment on lines
+17
to
+22
| # Naming conventions used by the emitter: | ||
| # * States `x[i]` -> `x{i}` (declared in `states = ...`) | ||
| # * Outputs `Y[i]` / `y[i]` -> `outeq_{i-1}` (declared in `outputs = ...`) | ||
| # * Inputs `b[j]`/`bolus[j]` -> `bolus(input_{j-1}) -> x{k}` (route) | ||
| # `rateiv[j]`/`r[j]`-> `infusion(input_{j-1}) -> x{k}`(route) | ||
| # * Params / covariates keep their (lower-cased) names. |
Comment on lines
+168
to
+178
| dsl_route_of <- function(expr) { | ||
| if (is.call(expr) && as.character(expr[[1]]) == "[") { | ||
| v <- tolower(as.character(expr[[2]])) | ||
| idx <- expr[[3]] | ||
| if (v %in% c("b", "bolus", "rateiv", "r") && is.numeric(idx) && length(idx) == 1) { | ||
| kind <- if (v %in% c("b", "bolus")) "bolus" else "infusion" | ||
| return(list(kind = kind, input = as.integer(idx))) | ||
| } | ||
| } | ||
| NULL | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 44 changed files in this pull request and generated 3 comments.
Files not reviewed (8)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
Comment on lines
+597
to
+604
| routes_result <- dsl_finalize_routes(eqn$routes) | ||
| routes <- routes_result$routes | ||
| # Pmetrics data uses 1-based INPUT labels. Bolus routes keep the data input | ||
| # label; infusion routes that share an input with a bolus receive a fresh | ||
| # label (see `dsl_finalize_routes`), captured in the remap table. | ||
| route_lines <- vapply(routes, function(r) { | ||
| sprintf("%s(input_%d) -> x%d", r$kind, r$label, r$comp) | ||
| }, character(1)) |
Comment on lines
+17
to
+22
| # Naming conventions used by the emitter: | ||
| # * States `x[i]` -> `x{i}` (declared in `states = ...`) | ||
| # * Outputs `Y[i]` / `y[i]` -> `outeq_{i-1}` (declared in `outputs = ...`) | ||
| # * Inputs `b[j]`/`bolus[j]` -> `bolus(input_{j-1}) -> x{k}` (route) | ||
| # `rateiv[j]`/`r[j]`-> `infusion(input_{j-1}) -> x{k}`(route) | ||
| # * Params / covariates keep their (lower-cased) names. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 46 changed files in this pull request and generated 2 comments.
Files not reviewed (9)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/PMbuild.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
Comment on lines
+92
to
+102
| // The output equation this error model applies to (1-based). Fall back to | ||
| // positional order if the field is absent, preserving old behaviour. | ||
| let outeq_1based = get_field(&em, "outeq") | ||
| .ok() | ||
| .and_then(|v| v.as_real()) | ||
| .map(|v| v as usize) | ||
| .unwrap_or(i + 1); | ||
| if outeq_1based < 1 { | ||
| bail!("error_models[{}].outeq must be 1 or greater", i + 1); | ||
| } | ||
| let outeq = outeq_1based - 1; |
Typically improves performance in the range of 10-30% in my testing
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 46 changed files in this pull request and generated 5 comments.
Files not reviewed (9)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/PMbuild.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
Comment on lines
+19
to
+21
| # * Outputs `Y[i]` / `y[i]` -> `outeq_{i-1}` (declared in `outputs = ...`) | ||
| # * Inputs `b[j]`/`bolus[j]` -> `bolus(input_{j-1}) -> x{k}` (route) | ||
| # `rateiv[j]`/`r[j]`-> `infusion(input_{j-1}) -> x{k}`(route) |
| extendr-api = "=0.9.0" | ||
| pmcore = { version = "=0.25.2", features = ["exa"] } | ||
| libloading = "0.9" | ||
| pmcore = { version = "0.26.0", features = ["dsl-jit"] } |
Comment on lines
+94
to
+98
| let outeq_1based = get_field(&em, "outeq") | ||
| .ok() | ||
| .and_then(|v| v.as_real()) | ||
| .map(|v| v as usize) | ||
| .unwrap_or(i + 1); |
Comment on lines
+597
to
+604
| routes_result <- dsl_finalize_routes(eqn$routes) | ||
| routes <- routes_result$routes | ||
| # Pmetrics data uses 1-based INPUT labels. Bolus routes keep the data input | ||
| # label; infusion routes that share an input with a bolus receive a fresh | ||
| # label (see `dsl_finalize_routes`), captured in the remap table. | ||
| route_lines <- vapply(routes, function(r) { | ||
| sprintf("%s(input_%d) -> x%d", r$kind, r$label, r$comp) | ||
| }, character(1)) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Collaborator
Author
|
@Siel and @mnneely, with the new model format and DSL, neither The same goes for While numeric inputs still work, they are silently converted to |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 47 changed files in this pull request and generated 3 comments.
Files not reviewed (9)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/PMbuild.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
Comments suppressed due to low confidence (1)
R/model_dsl.R:178
dsl_route_of()does not treat parentheses as transparent. If a route term is parenthesized (e.g.dx[1] <- ... + (rateiv[1])), it won’t be detected/stripped andexpr_to_dsl()will abort when it later seesrateiv[...]inside an expression. Unwrapping(here avoids this false-negative.
dsl_route_of <- function(expr) {
if (is.call(expr) && as.character(expr[[1]]) == "[") {
v <- tolower(as.character(expr[[2]]))
idx <- expr[[3]]
if (v %in% c("b", "bolus", "rateiv", "r") && is.numeric(idx) && length(idx) == 1) {
kind <- if (v %in% c("b", "bolus")) "bolus" else "infusion"
return(list(kind = kind, input = as.integer(idx)))
}
}
NULL
}
Comment on lines
6
to
10
| \usage{ | ||
| simulate_one(data_path, model_path, spp, kind) | ||
| simulate_one(data_path, model_source, spp) | ||
| } | ||
| \arguments{ | ||
| \item{data_path}{Path to the data file.} |
Comment on lines
6
to
10
| \usage{ | ||
| simulate_all(data_path, model_path, theta, kind) | ||
| simulate_all(data_path, model_source, theta) | ||
| } | ||
| \arguments{ | ||
| \item{data_path}{Path to the data file.} |
Comment on lines
6
to
12
| \usage{ | ||
| fit(model_path, data, params, output_path, kind) | ||
| fit(model_source, data, params, output_path) | ||
| } | ||
| \arguments{ | ||
| \item{model_path}{Path to the compiled model file.} | ||
| \item{model_source}{Model definition written in the pharmsol DSL.} | ||
|
|
||
| \item{data}{Path to the data file.} |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 47 changed files in this pull request and generated no new comments.
Files not reviewed (9)
- man/PM_build.Rd: Generated file
- man/PM_model.Rd: Generated file
- man/PMbuild.Rd: Generated file
- man/additive.Rd: Generated file
- man/fit.Rd: Generated file
- man/model_parameters.Rd: Generated file
- man/proportional.Rd: Generated file
- man/simulate_all.Rd: Generated file
- man/simulate_one.Rd: Generated file
Comments suppressed due to low confidence (4)
R/model_dsl.R:178
dsl_route_of()does not treat parentheses as transparent. A valid additive route term likedx[1] <- ... + (rateiv[1])will not be recognized/stripped as a route, and thenexpr_to_dsl()will abort when it encountersrateiv[...]/r[...]indexing. Unwrapping(calls here avoids that false-negative without changing semantics.
# If `expr` is a route reference (`b[j]`, `bolus[j]`, `rateiv[j]`, `r[j]`),
# return `list(kind = "bolus"|"infusion", input = j)`, else NULL.
dsl_route_of <- function(expr) {
if (is.call(expr) && as.character(expr[[1]]) == "[") {
v <- tolower(as.character(expr[[2]]))
idx <- expr[[3]]
if (v %in% c("b", "bolus", "rateiv", "r") && is.numeric(idx) && length(idx) == 1) {
kind <- if (v %in% c("b", "bolus")) "bolus" else "infusion"
return(list(kind = kind, input = as.integer(idx)))
}
}
NULL
}
man/simulate_one.Rd:15
- The Rd documentation is out of sync with the exported function signature:
simulate_one()now accepts an optionalsolverargument (seeR/extendr-wrappers.R), but the generated.Rdomits it from \usage and \arguments. This will confuse users and breaks?simulate_oneas an accurate reference.
man/simulate_all.Rd:15 - The Rd documentation is out of sync with the exported function signature:
simulate_all()now accepts an optionalsolverargument (seeR/extendr-wrappers.R), but the generated.Rdomits it from \usage and \arguments.
man/fit.Rd:17 - The Rd documentation is out of sync with the exported function signature:
fit()now accepts an optionalsolverargument (seeR/extendr-wrappers.R), but the generated.Rdomits it from \usage and \arguments.
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.
Summary
Tests