Conversation
Add BestDose dose optimization feature ported from Pmetrics_rust bestdose branch: - R/PM_bestdose.R: PM_bestdose and PM_bestdose_problem R6 classes for Bayesian dose optimization with support for concentration and AUC targets - src/rust/src/bestdose_executor.rs: Rust backend for BestDose optimization using pmcore's BestDoseProblem with ODE model support - Updated lib.rs with bestdose, bestdose_prepare, bestdose_optimize exports - Updated extendr-wrappers.R with R-side wrapper functions - Updated NAMESPACE with PM_bestdose, PM_bestdose_problem, bestdose exports - Bumped pmcore dependency from 0.21.1 to 0.22.1 (required for bestdose) - Added libloading dependency for dynamic model loading - Added bestdose example data (past, prior, target CSVs) and test script - Fixed executor.rs mutability issue for pmcore 0.22.1 compatibility
There was a problem hiding this comment.
Pull request overview
Adds BestDose dose-optimization support to the Pmetrics R package by introducing new R6 interfaces and wiring them to a Rust backend (ported from Pmetrics_rust#bestdose), along with docs and example inputs. This also bumps the Rust pmcore dependency to enable the new capability.
Changes:
- Add Rust BestDose executor + extendr exports (
bestdose, plus a prepare/optimize handle workflow). - Add R6 classes
PM_bestdose/PM_bestdose_problemand R wrappers to drive the Rust backend. - Add roxygen-generated man pages and example CSV/script assets; bump
pmcoreto0.22.1and addlibloading.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rust/src/lib.rs | Adds BestDose-related extendr exports and module wiring. |
| src/rust/src/executor.rs | Minor adjustment to fit result mutability. |
| src/rust/src/bestdose_executor.rs | New Rust implementation for BestDose prepare/optimize and R conversions. |
| src/rust/Cargo.toml | Bumps pmcore to 0.22.1 and adds libloading. |
| Cargo.lock | Lockfile updates from dependency bump/additions. |
| R/extendr-wrappers.R | Adds R-level .Call() wrappers for BestDose functions. |
| R/PM_bestdose.R | New R6 classes and helpers for BestDose workflows. |
| NAMESPACE | Exports new R6 classes and bestdose. |
| man/bestdose.Rd | New generated documentation for bestdose(). |
| man/PM_bestdose.Rd | New generated documentation for PM_bestdose. |
| man/PM_bestdose_problem.Rd | New generated documentation for PM_bestdose_problem. |
| inst/Examples/src/bestdose_*.csv | Example prior/past/target datasets. |
| inst/Examples/Rscript/bestdose_simple_test.R | Example script demonstrating reusable problem workflow. |
| .gitignore | Ensures example assets and example test scripts are not ignored. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Self { | ||
| id: id.to_string(), | ||
| time: pred.time(), | ||
| observed: pred.obs().unwrap_or(0.0), | ||
| pop_mean: pred.pop_mean(), | ||
| pop_median: pred.pop_median(), |
There was a problem hiding this comment.
observed is set to 0.0 when the prediction has no observation (pred.obs() is None). This will silently turn missing observations into real zeros in the returned data.frame. Prefer representing missing as NA by changing the field type (e.g., Option<f64>/Rfloat) and passing through pred.obs() without coercion.
| let pred_rows: Vec<BestDosePredictionRow> = result | ||
| .predictions() | ||
| .predictions() | ||
| .iter() | ||
| .map(|p| BestDosePredictionRow::from_np_prediction(p, "subject_1")) | ||
| .collect(); |
There was a problem hiding this comment.
The predictions data.frame hardcodes id to "subject_1" for all rows. This will mislabel results when the subject id is not exactly that string, and makes multi-subject extensions harder. Thread the actual subject id through from the problem/result (or at least use the target_data subject id used to build the problem) when constructing BestDosePredictionRows.
|
|
||
| #' Run BestDose optimization to find optimal doses | ||
| #'@export | ||
| bestdose <- function(model_path, prior_path, past_data_path, target_data_path, time_offset, dose_min, dose_max, bias_weight, target_type, params, kind) .Call(wrap__bestdose, model_path, prior_path, past_data_path, target_data_path, time_offset, dose_min, dose_max, bias_weight, target_type, params, kind) |
There was a problem hiding this comment.
bestdose() is exported to users, but the Rust backend returns errors as a character string (not an R error). The wrapper also does no is.character()/cli_abort() check, so failures may look like successful returns. Either make the Rust function return Result<...> so extendr raises an R error, or add an error check in the R wrapper and cli::cli_abort() when a character error is returned.
| bestdose <- function(model_path, prior_path, past_data_path, target_data_path, time_offset, dose_min, dose_max, bias_weight, target_type, params, kind) .Call(wrap__bestdose, model_path, prior_path, past_data_path, target_data_path, time_offset, dose_min, dose_max, bias_weight, target_type, params, kind) | |
| bestdose <- function(model_path, prior_path, past_data_path, target_data_path, time_offset, dose_min, dose_max, bias_weight, target_type, params, kind) { | |
| res <- .Call( | |
| wrap__bestdose, | |
| model_path, | |
| prior_path, | |
| past_data_path, | |
| target_data_path, | |
| time_offset, | |
| dose_min, | |
| dose_max, | |
| bias_weight, | |
| target_type, | |
| params, | |
| kind | |
| ) | |
| if (is.character(res)) { | |
| cli::cli_abort(res) | |
| } | |
| res | |
| } |
| model_for_settings <- if (!is.null(model_info$model)) model_info$model else model | ||
| settings <- bestdose_default_settings(prior, model_for_settings) |
There was a problem hiding this comment.
When model is provided as a compiled model path (character), bestdose_parse_model() sets model_info$model <- NULL, but if settings is also NULL this code will pass a character string into bestdose_default_settings(), which expects a PM_model and will error. Either require settings when model is a path, or implement a way to derive default ranges/settings from the compiled model metadata.
| model_for_settings <- if (!is.null(model_info$model)) model_info$model else model | |
| settings <- bestdose_default_settings(prior, model_for_settings) | |
| if (is.null(model_info$model)) { | |
| cli::cli_abort( | |
| "When 'model' is provided as a compiled model path, 'settings' must be supplied explicitly." | |
| ) | |
| } | |
| settings <- bestdose_default_settings(prior, model_info$model) |
| self$model_info <- model_info | ||
| self$settings <- settings | ||
|
|
||
| cli::cli_alert_success("BestDose problem prepared with %d support points", dim[1]) |
There was a problem hiding this comment.
cli::cli_alert_success() uses glue-style interpolation, not printf-style formatting. As written, "%d" will be printed literally. Use glue syntax (e.g., "... {dim[1]} ...") or pass a named argument for interpolation.
| cli::cli_alert_success("BestDose problem prepared with %d support points", dim[1]) | |
| cli::cli_alert_success( | |
| "BestDose problem prepared with {n_support} support points", | |
| n_support = dim[1] | |
| ) |
| #' @export | ||
| PM_bestdose_problem <- R6::R6Class( |
There was a problem hiding this comment.
New exported user-facing functionality (PM_bestdose, PM_bestdose_problem, and bestdose) is introduced without any testthat coverage. Since this repo already uses testthat, add at least basic tests for argument validation/error paths and (if feasible) a small end-to-end smoke test using the included example data (or a skipped test when compilation/runtime prerequisites aren’t available).
| let (library, (eq, meta)) = | ||
| unsafe { pmcore::prelude::pharmsol::exa::load::load::<ODE>(model_path) }; | ||
|
|
||
| let settings = settings(params, meta.get_params(), "/tmp/bestdose") | ||
| .map_err(|e| format!("Failed to parse settings: {}", e))?; | ||
|
|
There was a problem hiding this comment.
settings(params, meta.get_params(), "/tmp/bestdose") hardcodes a Unix-specific path and reuses the same directory across runs. This can break on Windows and can cause collisions when multiple BestDose problems are prepared concurrently. Use a per-run temp directory (e.g., std::env::temp_dir() + unique subdir) or accept an output/log directory from R and pass it through here.
Port BestDose from Pmetrics_rust#bestdose. Adds PM_bestdose/PM_bestdose_problem R6 classes, Rust backend, example data, and docs. Bumps pmcore to 0.22.1.