wip: add selected updates from BD-Michael - #375
Conversation
There was a problem hiding this comment.
Pull request overview
This PR is a work-in-progress integration of a new BestDose workflow into the Pmetrics codebase, adding R-side user APIs (R6 classes, plotting, and report generation) and scaffolding for a Rust backend intended to be exposed to R via extendr.
Changes:
- Adds new exported R6 classes
bdandbd_postplus helpers to parse inputs, run optimization, simulate, plot, and generate an HTML report. - Introduces a new Rust BestDose executor module and adds R
.Call()wrappers intended to bind to new extendr exports. - Updates package wiring (NAMESPACE, pkgdown reference, and generated man pages) and changes Rust default log level to WARN+.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/helper-models.R | Removes EXA temp cleanup logic (cleanup helper is now a no-op). |
| src/rust/src/lib.rs | Adjusts logging default level and adds comments near the extendr export module. |
| src/rust/src/bestdose_executor.rs | Adds a new Rust BestDose executor implementation (posterior prep + optimize + R conversion helpers). |
| R/PM_bestdose.R | Adds the main BestDose R6 APIs (bd, bd_post) and plotting implementation. |
| R/PM_bestdose_report.R | Adds BestDose report-building utilities and HTML report generation. |
| R/extendr-wrappers.R | Adds new .Call() wrappers for bestdose_prepare / bestdose_optimize. |
| NAMESPACE | Exports bd / bd_post and registers plot.bd, plus dplyr import tweaks. |
| man/plot.bd.Rd | Generated documentation for plot.bd. |
| man/bd.Rd | Generated documentation for bd. |
| man/bd_post.Rd | Generated documentation for bd_post. |
| _pkgdown.yml | Adds pkgdown reference entries for BestDose and an “Internal” section. |
Files not reviewed (3)
- man/bd.Rd: Generated file
- man/bd_post.Rd: Generated file
- man/plot.bd.Rd: Generated file
Suppressed comments (2)
src/rust/src/lib.rs:257
- The BestDose R wrappers call
.Call(wrap__bestdose_prepare)/.Call(wrap__bestdose_optimize), but this crate doesn't compile or export any corresponding#[extendr]functions: there is nomod bestdose_executor;andextendr_module!doesn't listbestdose_prepare/bestdose_optimize. As-is, the new BestDose API will fail at runtime because the symbols are never registered with R.
// Macro to generate exports.
// This ensures exported functions are registered with R.
// See corresponding C code in `entrypoint.c`.
extendr_module! {
mod Pmetrics;
fn simulate_one;
fn simulate_all;
fn fit;
fn model_parameters;
fn model_metadata;
fn setup_logs;
}
src/rust/src/bestdose_executor.rs:146
- Using
unwrap()onpath.to_str()will panic on non-UTF8 paths; return an error instead so R callers get a controlled failure.
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();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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))?; |
| let (population_theta, prior_weights) = | ||
| parse_prior(&prior_path.to_str().unwrap().to_string(), &settings) | ||
| .map_err(|e| format!("Failed to parse prior: {}", e))?; |
| Self { | ||
| id: id.to_string(), | ||
| time: pred.time(), | ||
| observed: pred.obs().unwrap_or(0.0), | ||
| pop_mean: pred.pop_mean(), |
| # Print target/future data for debugging | ||
| if (!quiet) { | ||
| cli::cli_h3("Target/Future data (for debugging)") |
| bd <- R6::R6Class( | ||
| "bd", | ||
| public = list( |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (19.69%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
- Coverage 61.80% 58.25% -3.56%
==========================================
Files 51 53 +2
Lines 22414 24460 +2046
==========================================
+ Hits 13853 14248 +395
- Misses 8561 10212 +1651
🚀 New features to boost your workflow:
|
|
@R-garreau and @Siel, I’m a bit confused. Are we only merging this to create a BestDose API in Pmetrics? Does the Shiny app need it? Does it still need debugging for posterior issues? |
|
Hi prof @mnneely, As I understand the shiny app is still using BD-Michael. This branch creates an interface to use BestDose in Pmetrics and Romain was using it for his app. The new rust desktop app is skipping Pmetrics so merging this would not be needed for that. Regarding the posterior issues @R-garreau's PR was merged, so I supposed it should be solved but more testing is needed for sure. |
|
That’s my understanding. This branch originated in BD-Michael -> BD-Michael2 as I tried to resolve conflicts and avoid overwriting main. I ended up copying main and only adding the bestdose specific files from BD-Michael. However, there are probably still issues on the rust side. I guess we need to decide if we want a bestdose API in Pmetrics. The advantage I could see would be if we enable bulk dose optimization for study purposes, e.g. the past.csv and future.csv files can have multiple subjects/targets and results are aggregated in the return object. Does anyone know if the current BD-Michael branch had resolved the posterior problems? |
This branch is a copy of main as of 13 August, with selected merging of files from BD-Michael.