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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let (library, (eq, meta)) = | ||
| unsafe { pmcore::prelude::pharmsol::exa::load::load::<ODE>(model_path) }; | ||
|
|
||
| let settings = settings(params, meta.get_params(), "/tmp/bestdose") |
There was a problem hiding this comment.
settings(params, meta.get_params(), "/tmp/bestdose") hard-codes a Unix-only temp directory and a fixed path. This is likely to fail on Windows and can cause cross-run collisions if multiple optimizations run concurrently. Consider using std::env::temp_dir() (or reusing the existing build::temp_path()/temporary_path() mechanism) and creating a unique per-run subdirectory, or allow the output path to be provided from R.
| let settings = settings(params, meta.get_params(), "/tmp/bestdose") | |
| // Use a per-run unique temporary directory instead of a hard-coded Unix path. | |
| let mut tmp_dir = std::env::temp_dir(); | |
| tmp_dir.push("bestdose"); | |
| let unique_subdir = format!( | |
| "run-{}-{}", | |
| std::process::id(), | |
| std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .map_err(|e| format!("System time error: {}", e))? | |
| .as_millis() | |
| ); | |
| tmp_dir.push(unique_subdir); | |
| std::fs::create_dir_all(&tmp_dir) | |
| .map_err(|e| format!("Failed to create temp directory {:?}: {}", tmp_dir, e))?; | |
| let tmp_dir_str = tmp_dir | |
| .to_str() | |
| .ok_or_else(|| "Temporary directory path is not valid UTF-8".to_string())?; | |
| let settings = settings(params, meta.get_params(), tmp_dir_str) |
| id: id.to_string(), | ||
| time: pred.time(), | ||
| observed: pred.obs().unwrap_or(0.0), | ||
| pop_mean: pred.pop_mean(), |
There was a problem hiding this comment.
BestDosePredictionRow::from_np_prediction converts a missing observation (pred.obs() == None) into 0.0. This changes the meaning of the data and can be indistinguishable from a true zero observation in R. Prefer representing missingness as NA (e.g., use an Option<f64> field if supported by IntoDataFrameRow, or use an Rfloat/nullable type that can encode NA) so downstream plotting/summary doesn’t misinterpret it.
| \name{PM_bestdose_problem} | ||
| \alias{PM_bestdose_problem} | ||
| \title{Prepare a reusable BestDose optimization problem} | ||
| \description{ | ||
| \ifelse{html}{\href{https://lifecycle.r-lib.org/articles/stages.html#experimental}{\figure{lifecycle-experimental.svg}{options: alt='[Experimental]'}}}{\strong{[Experimental]}} | ||
|
|
||
| Use \code{PM_bestdose_problem} to mirror the Rust workflow: compute posterior | ||
| support points once, inspect them in R, and solve for multiple bias weights | ||
| without repeating the expensive initialization step. | ||
| } |
There was a problem hiding this comment.
This Rd file documents PM_bestdose_problem, but the PR adds/exports PM_bestdose_posterior (and there is no PM_bestdose_problem object defined in R/PM_bestdose.R). This mismatch will confuse users and can trigger package check warnings. Either rename/regenerate the documentation to match the actual class name, or add the missing PM_bestdose_problem class and export it consistently.
| #' @title | ||
| #' Compute a reusable BestDose posterior | ||
| #' | ||
| #' @description | ||
| #' `r lifecycle::badge("experimental")` | ||
| #' | ||
| #' Use `PM_bestdose_posterior` to compute the Bayesian posterior once from | ||
| #' prior population data and patient history, then call `$optimize()` multiple | ||
| #' times with different targets, dose ranges, or bias weights. | ||
| #' | ||
| #' @export | ||
| PM_bestdose_posterior <- R6::R6Class( | ||
| "PM_bestdose_posterior", | ||
| public = list( | ||
| handle = NULL, | ||
| theta = NULL, | ||
| theta_dim = NULL, | ||
| param_names = NULL, | ||
| posterior_weights = NULL, | ||
| population_weights = NULL, | ||
| model_info = NULL, | ||
| settings = NULL, | ||
| initialize = function(prior, | ||
| model, | ||
| past_data = NULL, | ||
| max_cycles = 500, | ||
| settings = NULL) { | ||
| prior_path <- bestdose_parse_prior(prior) | ||
| model_info <- bestdose_parse_model(model) | ||
| past_data_path <- if (!is.null(past_data)) bestdose_parse_data(past_data) else NULL | ||
|
|
||
| if (is.null(settings)) { | ||
| model_for_settings <- if (!is.null(model_info$model)) model_info$model else model | ||
| settings <- bestdose_default_settings(prior, model_for_settings, max_cycles = max_cycles) | ||
| } | ||
|
|
||
| prep <- bestdose_prepare( | ||
| model_path = model_info$path, | ||
| prior_path = prior_path, | ||
| past_data_path = past_data_path, | ||
| params = settings, | ||
| kind = model_info$kind | ||
| ) | ||
|
|
||
| if (is.character(prep)) { | ||
| cli::cli_abort(prep) | ||
| } | ||
|
|
||
| dim <- as.integer(prep$theta_dim) | ||
| theta_matrix <- matrix(prep$theta_values, nrow = dim[1], ncol = dim[2]) | ||
| colnames(theta_matrix) <- prep$param_names | ||
|
|
||
| self$handle <- prep$handle | ||
| self$theta <- theta_matrix | ||
| self$theta_dim <- dim | ||
| self$param_names <- prep$param_names | ||
| self$posterior_weights <- prep$posterior_weights | ||
| self$population_weights <- prep$population_weights | ||
| self$model_info <- model_info | ||
| self$settings <- settings | ||
|
|
||
| cli::cli_alert_success("BestDose posterior computed with {dim[1]} support points") | ||
| }, | ||
| finalize = function() { | ||
| self$handle <- NULL | ||
| }, | ||
|
|
||
| #' @description | ||
| #' Run optimization and return raw list (doses, objf, predictions) | ||
| optimize_raw = function(target, | ||
| dose_range = list(min = 0, max = 1000), | ||
| bias_weight = 0.5, | ||
| target_type = "concentration", | ||
| time_offset = NULL) { | ||
| private$.run_optimize(target, dose_range, bias_weight, target_type, time_offset) | ||
| }, | ||
|
|
||
| #' @description | ||
| #' Run optimization and return a `PM_bestdose` result object | ||
| optimize = function(target, | ||
| dose_range = list(min = 0, max = 1000), | ||
| bias_weight = 0.5, | ||
| target_type = "concentration", | ||
| time_offset = NULL) { | ||
| raw <- self$optimize_raw(target, dose_range, bias_weight, target_type, time_offset) | ||
| PM_bestdose$new( | ||
| result = raw$result, | ||
| posterior_obj = self, | ||
| bias_override = raw$bias_weight | ||
| ) | ||
| } | ||
| ), |
There was a problem hiding this comment.
BestDose introduces substantial new user-facing behavior (posterior preparation + optimization) but there are currently no testthat tests exercising these code paths (only an example script under inst/Examples). Given the repo already has a tests/testthat suite, consider adding at least one automated test that runs a minimal BestDose workflow and asserts basic invariants (e.g., returns a list with expected fields, validates bias_weight bounds, and errors cleanly on invalid target_type).
| let past_data = if let Some(path) = past_data_path { | ||
| let data = data::read_pmetrics(path.to_str().unwrap()) | ||
| .map_err(|e| format!("Failed to read past data: {}", e))?; | ||
| let subjects = data.subjects(); | ||
| if subjects.is_empty() { | ||
| return Err("Past data file contains no subjects".to_string()); | ||
| } | ||
| Some(subjects[0].clone()) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let posterior = BestDosePosterior::compute( | ||
| &population_theta, | ||
| &population_weights, | ||
| past_data, | ||
| eq, | ||
| settings, | ||
| ) | ||
| .map_err(|e| format!("Failed to compute BestDose posterior: {}", e))?; | ||
|
|
||
| Ok(Self { posterior, library }) | ||
| } | ||
|
|
||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn optimize( | ||
| &self, | ||
| target_data_path: PathBuf, | ||
| time_offset: Option<f64>, | ||
| dose_min: f64, | ||
| dose_max: f64, | ||
| bias_weight: f64, | ||
| target_type: &str, | ||
| ) -> std::result::Result<BestDoseResult, String> { | ||
| let target_data = { | ||
| let data = data::read_pmetrics(target_data_path.to_str().unwrap()) | ||
| .map_err(|e| format!("Failed to read target data: {}", e))?; | ||
| let subjects = data.subjects(); | ||
| if subjects.is_empty() { | ||
| return Err("Target data file contains no subjects".to_string()); | ||
| } | ||
| subjects[0].clone() | ||
| }; |
There was a problem hiding this comment.
past_data/target_data are loaded and then subjects[0] is used without checking whether the file contains multiple subjects. This silently ignores additional subjects and makes results dependent on file ordering. If BestDose is intended to be single-subject only, consider validating subjects.len() == 1 and returning a clear error (or add an argument to select the subject by ID).
| setup_logs <- function() .Call(wrap__setup_logs) | ||
|
|
||
| #' Run BestDose optimization to find optimal doses | ||
| #'@export |
There was a problem hiding this comment.
Roxygen tags in this file consistently use #' @export with a space (e.g. setup_logs). Here #'@export is inconsistent and can be brittle for tooling/linters. Align it to the existing style (#' @export).
| #'@export | |
| #' @export |
| pmcore = { path = "../../../PMcore", features = ["exa"] } | ||
| # pmcore = { version = "=0.22.1", features = ["exa"] } | ||
| libloading = "0.9" |
There was a problem hiding this comment.
pmcore is switched from a crates.io version pin to a relative path dependency. This will break builds in CI/CRAN and for any user who doesn’t have a ../../../PMcore checkout; it also contradicts the PR description ("bumps pmcore to 0.22.1"). Prefer restoring a versioned dependency (e.g. =0.22.1) and, if local development needs an override, use a workspace/[patch.crates-io] setup that isn’t committed for release builds.
| ) | ||
|
|
||
| if (is.character(prep)) { | ||
| cli::cli_abort(prep) |
There was a problem hiding this comment.
cli::cli_abort(prep) is called with a message string that comes directly from the Rust backend and may contain user-controlled values (e.g., settings$algorithm or settings$prior). Because cli treats the entire string as a glue template, any {} braces in that error message can cause arbitrary R expressions to be evaluated, leading to code execution if an attacker can influence those fields. To fix this, treat the backend error as data rather than a template (e.g., pass it as a variable in a static cli message or strip/escape braces before calling cli::cli_abort).
| cli::cli_abort(prep) | |
| cli::cli_abort("BestDose backend error: {msg}", msg = prep) |
| target_type | ||
| ) | ||
| if (is.character(res)) { | ||
| cli::cli_abort(res) |
There was a problem hiding this comment.
cli::cli_abort(res) is invoked on the raw error string returned from bestdose_optimize, which ultimately wraps errors from BestDosePosteriorHandle::optimize and the underlying pmcore/data parsers. Those errors can embed user-controlled values (e.g., malformed settings entries or CSV paths containing {...}), and because cli interprets {} as glue expressions, this can be abused to execute arbitrary R code when optimization fails on attacker-controlled input. The handler should instead log or display the error as plain text (for example, by interpolating it into a static cli template or escaping braces) so that user data is never parsed as a glue expression.
| cli::cli_abort(res) | |
| cli::cli_abort("BestDose optimization failed: {msg}", msg = res) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raw <- posterior$optimize_raw( | ||
| target = target, | ||
| dose_range = dose_range, | ||
| bias_weight = bias_weight, | ||
| target_type = target_type, | ||
| time_offset = time_offset | ||
| ) | ||
| private$.set_result(raw$result, posterior, raw$bias_weight) |
There was a problem hiding this comment.
PM_bestdose$initialize() calls posterior$optimize_raw(), but PM_bestdose_posterior does not define an optimize_raw method anywhere in this file. As written, constructing PM_bestdose$new(...) without a precomputed result will error immediately. Add the missing optimize_raw() method (likely a thin wrapper around private$.run_optimize(...)) or update the call site to use the existing optimize() method and adjust how the result is stored.
| raw <- posterior$optimize_raw( | |
| target = target, | |
| dose_range = dose_range, | |
| bias_weight = bias_weight, | |
| target_type = target_type, | |
| time_offset = time_offset | |
| ) | |
| private$.set_result(raw$result, posterior, raw$bias_weight) | |
| posterior$optimize( | |
| target = target, | |
| dose_range = dose_range, | |
| bias_weight = bias_weight, | |
| target_type = target_type, | |
| time_offset = time_offset | |
| ) | |
| private$.set_result(posterior$result, posterior, posterior$bias_weight) |
| print = function() { | ||
| cat("BestDose Optimization Results\n") | ||
| cat("==============================\n\n") | ||
| cat(sprintf("Optimal doses: [%.2f, %.2f] mg\n", self$get_doses()[1], self$get_doses()[2])) | ||
| cat(sprintf("Objective function: %.10f\n", self$get_objf())) | ||
| cat(sprintf("ln(Objective): %.4f\n", log(self$get_objf()))) | ||
| cat(sprintf("Method: %s\n", self$get_method())) | ||
| cat(sprintf("Status: %s\n", self$get_status())) | ||
| if (!is.null(self$bias_weight)) { | ||
| cat(sprintf("Bias weight (lambda): %.2f\n", self$bias_weight)) | ||
| } | ||
| cat(sprintf("\nNumber of predictions: %d\n", nrow(self$result$predictions))) | ||
| if (!is.null(self$result$auc_predictions)) { | ||
| cat(sprintf("Number of AUC predictions: %d\n", nrow(self$result$auc_predictions))) | ||
| } |
There was a problem hiding this comment.
print() assumes there are exactly two doses (self$get_doses()[1] and [2]). If BestDose returns a different number of dose values (or if optimization fails partially), this will error during printing. Consider printing a variable-length vector safely (e.g., collapse/format all doses) or validating the expected length before indexing.
| % Generated by roxygen2: do not edit by hand | ||
| % Please edit documentation in R/PM_bestdose.R | ||
| \name{PM_bestdose_problem} | ||
| \alias{PM_bestdose_problem} | ||
| \title{Prepare a reusable BestDose optimization problem} | ||
| \description{ | ||
| \ifelse{html}{\href{https://lifecycle.r-lib.org/articles/stages.html#experimental}{\figure{lifecycle-experimental.svg}{options: alt='[Experimental]'}}}{\strong{[Experimental]}} | ||
|
|
||
| Use \code{PM_bestdose_problem} to mirror the Rust workflow: compute posterior | ||
| support points once, inspect them in R, and solve for multiple bias weights | ||
| without repeating the expensive initialization step. | ||
| } | ||
| \section{Methods}{ | ||
| \subsection{Public methods}{ | ||
| \itemize{ | ||
| \item \href{#method-PM_bestdose_problem-new}{\code{PM_bestdose_problem$new()}} | ||
| \item \href{#method-PM_bestdose_problem-finalize}{\code{PM_bestdose_problem$finalize()}} | ||
| \item \href{#method-PM_bestdose_problem-optimize_raw}{\code{PM_bestdose_problem$optimize_raw()}} | ||
| \item \href{#method-PM_bestdose_problem-optimize}{\code{PM_bestdose_problem$optimize()}} | ||
| \item \href{#method-PM_bestdose_problem-clone}{\code{PM_bestdose_problem$clone()}} |
There was a problem hiding this comment.
This man page documents PM_bestdose_problem, but there is no corresponding R6 class or exported object with that name (the code/NAMESPACE introduce PM_bestdose_posterior instead). R CMD check will likely report undocumented/mismatched topics. Either add the missing PM_bestdose_problem implementation/export, or rename/regenerate this documentation to match the actual class (PM_bestdose_posterior) and its real methods.
| @@ -0,0 +1,52 @@ | |||
| devtools::load_all() | |||
|
|
|||
| setwd("inst/Examples/Runs") | |||
There was a problem hiding this comment.
This example script changes the working directory to inst/Examples/Runs, but that directory is not present in the repository. Running the script as-is will fail at startup. Update the path to an existing directory (or create it as part of the examples) so the example is runnable.
| setwd("inst/Examples/Runs") | |
| setwd("inst/Examples/Rscript") |
| #' @export | ||
| PM_bestdose <- R6::R6Class( | ||
| "PM_bestdose", |
There was a problem hiding this comment.
New exported user-facing API (PM_bestdose / PM_bestdose_posterior) is added without any accompanying testthat coverage. Please add at least a minimal test that exercises construction/validation logic and confirms the optimize path returns the expected structured result (or a clear error) to prevent regressions.
| 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(), | ||
| post_mean: pred.post_mean(), | ||
| post_median: pred.post_median(), | ||
| outeq: pred.outeq(), |
There was a problem hiding this comment.
observed uses pred.obs().unwrap_or(0.0), which silently turns missing observations into 0.0 in the returned R data frame. That changes the meaning of the data and can break downstream plotting/metrics. Prefer representing missing values as NA (e.g., make observed an Option<f64>/nullable column or explicitly map to an R NA).
| .run_optimize = function(target, dose_range, bias_weight, target_type, time_offset) { | ||
| if (is.null(self$handle)) { | ||
| cli::cli_abort(c( x = "PM_bestdose_posterior object is not properly initialized", | ||
| "i" = "Re-run `PM_bestdose$new()`." )) |
There was a problem hiding this comment.
The initialization error hint says to re-run PM_bestdose$new(), but the object in question is PM_bestdose_posterior. This message is likely to confuse users troubleshooting initialization issues. Update the hint to reference PM_bestdose_posterior$new() (or the correct constructor/workflow).
| "i" = "Re-run `PM_bestdose$new()`." )) | |
| "i" = "Re-run `PM_bestdose_posterior$new()`." )) |
| pmcore = { git = "https://github.com/LAPKB/PMcore", branch = "feat/new-bestdose-api", features = ["exa"] } | ||
| # pmcore = { version = "=0.22.1", features = ["exa"] } | ||
| libloading = "0.9" |
There was a problem hiding this comment.
pmcore is pulled from a moving git branch (feat/new-bestdose-api) which makes builds non-reproducible and also conflicts with the PR description claiming a bump to 0.22.1 (Cargo.lock resolves pmcore as 0.22.2). Please pin to a published crates.io version (e.g. =0.22.1) or at least pin the git dependency to an immutable rev/tag so CI and downstream builds are stable.
| 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.
Hard-coding the settings output path to /tmp/bestdose is not portable (Windows/CRAN) and can fail if the directory doesn’t exist or isn’t writable. Please use the package’s temp/build path helper (e.g., the same temp path used elsewhere in the Rust code) and ensure the directory is created before writing settings/logs.
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.