wip: BD-michael2 - #374
Closed
mnneely wants to merge 47 commits into
Closed
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
Reworked bd and bd_post R6 classes: bd_post$optimize() now returns a bd object, and bd$new() supports one-shot usage. Removed dead Rust code (bestdose(), bestdose_ode(), bestdose_analytical()). Fixed bias_weight being NULL and print() breaking with variable dose counts. Updated PMcore dep to use relative time_offset gap semantics.
…sed most of the logs for bestdose, needs more work
…l some isues with the dose markers
…ata, or filename for future.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR rebases the BD-Michael branch onto main and introduces a new “BestDose” workflow, including R-facing APIs (R6 classes) and supporting Rust/R bindings, alongside substantial documentation and example updates.
Changes:
- Adds new BestDose user APIs in R (
bd,bd_post) plus plotting/docs and pkgdown reference updates. - Introduces a Rust BestDose executor layer and R
.Call()wrappers intended to bridge into Rust. - Updates options/utilities (e.g., date format handling, R release download helpers) and refreshes many
.Rddocs and example assets.
Reviewed changes
Copilot reviewed 34 out of 45 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rust/src/lib.rs | Adjusts logging defaults and the extendr export block (currently missing BestDose exports). |
| src/rust/src/bestdose_executor.rs | Adds Rust-side BestDose posterior/optimization implementation and R conversion helpers. |
| R/PMutilities.R | Adds latestR() and downloadR() helper functions. |
| R/PMoptions.R | Updates options UI/logic but currently contains unresolved merge-conflict markers. |
| R/PM_bestdose.R | Adds BestDose R6 classes (bd, bd_post) and plot.bd. |
| R/extendr-wrappers.R | Adds .Call() wrappers for BestDose prepare/optimize. |
| NAMESPACE | Adds exports/imports but currently contains unresolved merge-conflict markers. |
| man/simulate_one.Rd | Documentation tweak (removes internal keyword). |
| man/simulate_all.Rd | Documentation tweak (removes internal keyword). |
| man/PM_valid.Rd | Large roxygen output reformat/updates. |
| man/PM_result.Rd | Large roxygen output reformat/updates. |
| man/PM_post.Rd | Updates wording and pipe examples. |
| man/PM_pop.Rd | Updates wording and pipe examples. |
| man/PM_op.Rd | Updates wording and pipe examples. |
| man/PM_final.Rd | Updates wording and pipe examples. |
| man/PM_cycle.Rd | Updates wording and pipe examples. |
| man/PM_cov.Rd | Updates wording and pipe examples. |
| man/plot.PM_post.Rd | Updates argument docs; currently includes an Rd syntax issue. |
| man/plot.PM_pop.Rd | Updates argument docs; currently includes an Rd syntax issue. |
| man/plot.bd.Rd | Adds documentation for plot.bd. |
| man/model_parameters.Rd | Documentation tweak (removes internal keyword). |
| man/makeAUC.Rd | Replaces docs content; examples currently don’t match makeAUC(). |
| man/fit.Rd | Documentation tweak (removes internal keyword). |
| man/bd.Rd | Adds documentation for bd. |
| man/bd_post.Rd | Adds documentation for bd_post. |
| inst/options/PMoptions.json | Adds new option keys (e.g., backend, model_template_path). |
| inst/Examples/src/bestdose_target.csv | Adds BestDose example target data. |
| inst/Examples/src/bestdose_target_pmcore.csv | Adds BestDose example target data variant. |
| inst/Examples/src/bestdose_prior.csv | Adds BestDose example prior data. |
| inst/Examples/src/bestdose_prior_pmcore.csv | Adds BestDose example prior data variant. |
| inst/Examples/src/bestdose_past.csv | Adds BestDose example past data. |
| inst/Examples/Rscript/test_status_column.R | Adds BestDose example/test script. |
| inst/Examples/Rscript/test_future_vectors.R | Adds BestDose example/test script for vectorized future specs. |
| inst/Examples/Rscript/run_bestdose_comparison.R | Adds BestDose comparison script. |
| inst/Examples/Rscript/bestdose_simple_test.R | Adds BestDose simple test script. |
| .gitignore | Expands ignored paths (including examples/run artifacts). |
| _pkgdown.yml | Adds BestDose and “Internal” reference sections. |
Files not reviewed (5)
- man/PM_cov.Rd: Generated file
- man/PM_cycle.Rd: Generated file
- man/PM_final.Rd: Generated file
- man/PM_op.Rd: Generated file
- man/PM_pop.Rd: Generated file
Suppressed comments (3)
R/PMoptions.R:525
- Unresolved merge-conflict markers are present in the settings-restore logic, leaving dead/empty branches and invalid R code. Resolve the conflict and remove the markers so
update_check/update_timeouthandling is either retained or intentionally removed.
if (!is.null(settings$ic_method)) shiny::updateSelectInput(session, "ic_method", selected = settings$ic_method)
if (!is.null(settings$date_format)) shiny::updateSelectInput(session, "date_format", selected = settings$date_format)
<<<<<<< HEAD
if (!is.null(settings$update_check)) shiny::updateSelectInput(session, "update_check", selected = settings$update_check)
if (!is.null(settings$update_timeout)) shiny::updateNumericInput(session, "update_timeout", value = settings$update_timeout)
R/PMoptions.R:575
- This
bindEvent(...)argument list still contains merge-conflict markers, which makes the file invalid R. Resolve the conflict and remove the markers so the reactive change-tracking binds to the correct inputs.
input$sep, input$dec, input$digits, input$show_metrics,
input$bias_method, input$imp_method, input$use_percent,
input$ic_method, input$report_template, input$date_format,
<<<<<<< HEAD
input$update_check, input$update_timeout,
R/PMoptions.R:621
- The settings list being saved contains unresolved merge-conflict markers, making the file invalid and also potentially dropping persisted options. Resolve the conflict and remove markers so the settings list is syntactically correct and includes the intended fields.
settings <- list(
sep = input$sep,
dec = input$dec,
digits = input$digits,
show_metrics = input$show_metrics,
bias_method = glue::glue(c("", "percent_")[1 + as.numeric(input$use_percent)], input$bias_method),
imp_method = glue::glue(c("", "percent_")[1 + as.numeric(input$use_percent)], input$imp_method),
ic_method = input$ic_method,
report_template = input$report_template,
<<<<<<< HEAD
date_format = input$date_format,
update_check = input$update_check,
update_timeout = as.numeric(input$update_timeout)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+246
to
250
| // Macro to generate exports. | ||
| // This ensures exported functions are registered with R. | ||
| // See corresponding C code in `entrypoint.c`. | ||
| extendr_module! { | ||
| mod Pmetrics; |
Comment on lines
+130
to
+135
| 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))?; | ||
|
|
Comment on lines
245
to
249
| "date_format", | ||
| bslib::tooltip( | ||
| shiny::tags$span("Date format", shiny::icon("circle-question", class = "ms-1 text-muted")), | ||
| <<<<<<< HEAD | ||
| "Format used to parse date strings" |
Comment on lines
167
to
170
| importFrom(R6,R6Class) | ||
| <<<<<<< HEAD | ||
| importFrom(bslib, | ||
| accordion, |
| \code{list(color = "dodgerblue", width = 1, dash = "solid")}. | ||
| Markers can be joined by lines, default is \code{TRUE}. If \code{TRUE}, | ||
| the joining lines will have the following properties: | ||
| \verb{list(color = "dodgerblue", width = 1, dash = "solid"}. |
| If joined, | ||
| Markers can be joined by lines, default is \code{TRUE}. If \code{TRUE}, | ||
| the joining lines will have the following properties: | ||
| \verb{list(color = "dodgerblue", width = 1, dash = "solid"}. |
Comment on lines
+73
to
+84
| \examples{ | ||
| \dontrun{ | ||
| NPex$cov$plot(V ~ wt) | ||
| NPex$cov$plot(Ke ~ wt, line = list(lm = TRUE, ref = FALSE, loess = FALSE)) | ||
| NPex$cov$plot(Ke ~ wt, line = list(loess = list(ci = 0.9, color = "green"))) | ||
| NPex$cov$plot(V ~ time, marker = list(color = "blue")) | ||
| NPex$cov$plot(V ~ wt, | ||
| line = list(lm = TRUE, loess = FALSE), | ||
| stats = list(x = 0.5, y = 0.2, font = list(size = 7, color = "blue")) | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 37 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- man/PM_data.Rd: Generated file
- man/PM_sim.Rd: Generated file
- man/bd.Rd: Generated file
- man/bd_post.Rd: Generated file
- man/downloadR.Rd: Generated file
Suppressed comments (9)
src/rust/src/lib.rs:5
bestdose_executor.rswas added, but it is not compiled becauselib.rsdoes not declare the module (and still has a stray commentedmod build;). As-is, none of the BestDose Rust code is reachable from the crate, and the R BestDose API will fail once it tries to call into Rust.
src/rust/src/lib.rs:248bestdose_prepare()/bestdose_optimize()need#[extendr]wrappers in Rust so extendr can generate/resolvewrap__bestdose_prepareandwrap__bestdose_optimizefor the R layer. Right now there are no such exported functions inlib.rs.
src/rust/src/lib.rs:257- Even after adding
#[extendr]wrappers, the functions must be listed inextendr_module! { ... }so they’re registered and wrappers are generated consistently.
src/rust/src/bestdose_executor.rs:134 - Hard-coding the output directory to
/tmp/bestdoseis not portable (e.g., Windows) and can fail if/tmpis unavailable or not writable. Usingstd::env::temp_dir()avoids platform-specific paths.
man/latestR.Rd:10 - The generated Rd for
latestR()contains duplicated \usage/\value/\description blocks (each appears twice). This will produce confusing help output and should be regenerated/fixed so each section appears only once.
man/downloadR.Rd:10 - The generated Rd for
downloadR()contains duplicated \usage/\value/\description blocks (each appears twice). This should be regenerated/fixed so each section appears only once.
man/PM_data.Rd:33 - This doc now links users to deprecated aliases
makeNCA/makeAUCeven though their help pages state they are deprecated in favor ofmake_NCA/make_AUC. The documentation should reference the non-deprecated functions.
man/PM_sim.Rd:740 PM_sim$auc()documentation now says it forwards arguments tomakeAUC, butmakeAUC()is a deprecated alias (usemake_AUC()). Updating the link avoids directing users to a deprecated API.
R/PM_bestdose.R:16bd/bd_postare exported new user-facing APIs, but there are currently no testthat tests covering their input validation and key behaviors (e.g., future list recycling, start parsing, and dose replacement). The new example scripts underinst/Examples/are helpful, but they won’t run on CI by default.
#' @export
bd <- R6::R6Class(
"bd",
Comment on lines
+518
to
+520
| future_sim_data <- self$future$standard_data | ||
| n_future_doses <- sum(future_sim_data$evid != 0) | ||
| future_sim_data$dose[future_sim_data$evid != 0] <- tail(self$result$doses, n_future_doses) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 37 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- man/PM_data.Rd: Generated file
- man/PM_sim.Rd: Generated file
- man/bd.Rd: Generated file
- man/bd_post.Rd: Generated file
- man/downloadR.Rd: Generated file
Suppressed comments (5)
src/rust/src/lib.rs:4
- The new BestDose Rust implementation lives in
bestdose_executor.rs, but the module isn’t declared inlib.rs. Withoutmod bestdose_executor;, the file won’t be compiled and its types/functions can’t be referenced by exported wrappers.
src/rust/src/lib.rs:250 - R wrapper functions were added for
.Call(wrap__bestdose_prepare)/.Call(wrap__bestdose_optimize), but there are no corresponding#[extendr]exports registered inextendr_module!. This will fail at runtime with missing native symbols. Add#[extendr]wrapper functions and register them inextendr_module!.
man/latestR.Rd:10 latestR.Rdcontains duplicated \usage, \value, and \description blocks (each appears twice). This will trigger R CMD check warnings/errors for malformed Rd. Remove the duplicates (or regenerate docs) so each section appears once.
man/downloadR.Rd:10downloadR.Rdcontains duplicated \usage, \value, and \description blocks (each appears twice). This will trigger R CMD check warnings/errors for malformed Rd. Remove the duplicates (or regenerate docs) so each section appears once.
man/plot.PM_data.Rd:103- The
group_namesexample refers tocolor = "gender", but the argument controlling grouping isgroup(seeplot.PM_datasignature). Update the example to avoid confusing users.
Comment on lines
+58
to
+60
| bestdose_prepare <- function(model_path, prior_path, past_data_path, params, kind) .Call(wrap__bestdose_prepare, model_path, prior_path, past_data_path, params, kind) | ||
|
|
||
| bestdose_optimize <- function(handle, target_data_path, time_offset, dose_min, dose_max, bias_weight, target_type) .Call(wrap__bestdose_optimize, handle, target_data_path, time_offset, dose_min, dose_max, bias_weight, target_type) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 40 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- man/PM_data.Rd: Generated file
- man/PM_sim.Rd: Generated file
- man/bd.Rd: Generated file
Suppressed comments (9)
src/rust/src/lib.rs:5
- BestDose support is implemented in
bestdose_executor.rs, but the module is never declared inlib.rs. As a result, none of the BestDose code is compiled into the Rust library, and the corresponding R.Call(wrap__bestdose_*)wrappers will be unresolved at link/load time.
src/rust/src/lib.rs:248 - The R layer calls
.Call(wrap__bestdose_prepare)/.Call(wrap__bestdose_optimize), but there are no#[extendr]functions (and noextendr_module!registrations) for those entrypoints. This will prevent wrapper generation/registration and break BestDose calls at runtime.
man/PM_data.Rd:166 PM_data$auc()documentation refers tomakeAUC, butmakeAUC()is deprecated. Point these references tomake_AUCto avoid steering users toward a deprecated API.
man/PM_data.Rd:189PM_data$nca()documentation refers tomakeNCA, butmakeNCA()is deprecated. Point these references tomake_NCAto avoid steering users toward a deprecated API.
src/rust/src/bestdose_executor.rs:134settings(...)is called with 3 arguments here, butcrate::settings::settingsis defined assettings(List, params, outputs, output_path)(4 args). Oncebestdose_executoris wired intolib.rs, this will be a compile error and block building BestDose entirely.
man/PM_data.Rd:33- This section links to
makeNCA/makeAUC, but those functions are deprecated wrappers (seeR/Deprecated.R). The documentation should point users to the non-deprecated APIsmake_NCA/make_AUC.
This issue also appears in the following locations of the same file:
- line 160
- line 180
man/latestR.Rd:10
- The generated Rd has duplicated \usage, \value, and \description blocks (each appears twice). This will produce noisy/manually confusing documentation and can trigger R CMD check warnings.
man/downloadR.Rd:10 - The generated Rd has duplicated \usage, \value, and \description blocks (each appears twice). This should be deduplicated to avoid confusing docs and potential check warnings.
man/PM_sim.Rd:740 PM_sim$auc()documentation points users tomakeAUC, butmakeAUC()is deprecated. This should referencemake_AUCinstead.
Contributor
Author
|
Complex merge. Will try more restricted merge in #375 |
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.
This branch is BD-Michael rebased on main. Need to check carefully before merging into main to ensure nothing is overwritten in main that we do not want.