feat: new bestdose API#247
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new two-stage BestDose public API centered around BestDosePosterior (compute posterior once, then run dose optimization), and updates tests/examples to use the new entry point while making some internal modules/functions crate-private.
Changes:
- Replace
BestDoseProblempublic entry point withBestDosePosterior::{compute, optimize}. - Restrict internal APIs (
cost,predictions, and related helpers) topub(crate)visibility. - Update integration tests and examples to the new compute-then-optimize workflow.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/bestdose_tests.rs | Migrates integration tests to the BestDosePosterior API and adjusts assertions accordingly. |
| src/bestdose/types.rs | Adds BestDosePosterior type and updates docs to describe the two-stage API (needs doc fixes). |
| src/bestdose/predictions.rs | Makes final prediction helper crate-private. |
| src/bestdose/mod.rs | Re-exports BestDosePosterior, implements the new public two-stage API, and makes internal modules crate-private. |
| src/bestdose/cost.rs | Makes cost function crate-private. |
| examples/bestdose_bounds.rs | Updates example to compute posterior once and reuse it across optimizations. |
| examples/bestdose_auc.rs | Updates AUC examples to the two-stage API. |
| examples/bestdose.rs | Updates main example to use BestDosePosterior and loop optimizations by bias weight. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Re-export public API | ||
| pub use types::{BestDoseProblem, BestDoseResult, DoseRange, Target}; | ||
| pub use types::{BestDosePosterior, BestDoseResult, DoseRange, Target}; | ||
|
|
There was a problem hiding this comment.
The module-level docs still reference BestDoseProblem as a public “main entry point” and discuss parameters like max_cycles, but the public re-export now only exposes BestDosePosterior. Please update the surrounding docs to reflect the two-stage API so generated documentation doesn’t point users at removed types/parameters.
| // Use current_time to separate past and future | ||
| let problem = BestDoseProblem::new( | ||
| let posterior = BestDosePosterior::compute( | ||
| &prior_theta, | ||
| &prior_weights, | ||
| Some(past), | ||
| eq.clone(), | ||
| settings.clone(), | ||
| )?; | ||
|
|
||
| let result = posterior.optimize( | ||
| target, | ||
| Some(2.0), // Current time = 2.0 hours | ||
| eq.clone(), | ||
| DoseRange::new(0.0, 500.0), |
There was a problem hiding this comment.
This test comment says the time_offset is used to “separate past and future”, but the new BestDosePosterior::optimize implementation currently ignores time_offset (it returns the target unchanged). Update the comment and/or pass None here to avoid implying behavior that isn’t present.
| /// # eq: pharmsol::prelude::ODE, | ||
| /// # error_models: pharmsol::prelude::ErrorModels, | ||
| /// # settings: pmcore::routines::settings::Settings) | ||
| /// # -> anyhow::Result<()> { | ||
| /// let problem = BestDoseProblem::new( | ||
| /// // Stage 1: Compute posterior (expensive, done once) | ||
| /// let posterior = BestDosePosterior::compute( | ||
| /// &population_theta, | ||
| /// &population_weights, | ||
| /// Some(past), // Patient history | ||
| /// target, // Dosing template with targets | ||
| /// Some(past), | ||
| /// eq, | ||
| /// error_models, | ||
| /// DoseRange::new(0.0, 1000.0), | ||
| /// 0.5, // Balanced personalization | ||
| /// settings, | ||
| /// 500, // NPAGFULL cycles | ||
| /// Target::Concentration, | ||
| /// )?; |
There was a problem hiding this comment.
The docs example still passes an error_models argument to BestDosePosterior::compute, but the new compute signature only takes (population_theta, population_weights, past_data, eq, settings). This makes the public docs misleading; update the example signature and the call to remove error_models (or reintroduce it in the API if that was intended).
| /// * `error_models` - Error model specifications | ||
| /// * `settings` - NPAG settings for posterior refinement | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust,no_run,ignore | ||
| /// let posterior = BestDosePosterior::compute( | ||
| /// &theta, &weights, | ||
| /// Some(past_subject), | ||
| /// eq, error_models, settings, |
There was a problem hiding this comment.
BestDosePosterior::compute no longer takes an error_models parameter, but the doc comment still lists it (and the example call uses it). Please update the argument list/example to match the actual signature and clarify that error models come from settings.errormodels.
| /// * `error_models` - Error model specifications | |
| /// * `settings` - NPAG settings for posterior refinement | |
| /// | |
| /// # Example | |
| /// | |
| /// ```rust,no_run,ignore | |
| /// let posterior = BestDosePosterior::compute( | |
| /// &theta, &weights, | |
| /// Some(past_subject), | |
| /// eq, error_models, settings, | |
| /// * `settings` - NPAG settings for posterior refinement (including error models via `settings.errormodels`) | |
| /// | |
| /// # Example | |
| /// | |
| /// ```rust,no_run,ignore | |
| /// // Error models should be configured as part of `settings.errormodels` | |
| /// let posterior = BestDosePosterior::compute( | |
| /// &theta, &weights, | |
| /// Some(past_subject), | |
| /// eq, settings, |
| let (posterior_theta, posterior_weights, filtered_population_weights, _past_subject) = | ||
| calculate_posterior_density( | ||
| population_theta, | ||
| population_weights, | ||
| past_data.as_ref(), | ||
| &eq, | ||
| &settings.errormodels, | ||
| &settings, | ||
| )?; |
There was a problem hiding this comment.
BestDosePosterior::compute discards the past_subject returned by calculate_posterior_density (_past_subject), but the optimization/cost simulation depends on dose history to set the correct state when optimizing future targets. Consider storing the past-dose history inside BestDosePosterior and using it in optimize() (e.g., via the existing prepare_target_subject/concatenation logic), otherwise users must manually rebuild a combined subject and results can be wrong for patients with drug-on-board.
| // The time_offset mode concatenates a dummy empty-past with the target. | ||
| let final_target = match time_offset { | ||
| None => target, | ||
| Some(t) => { | ||
| // When using time_offset without past data in the target itself, | ||
| // we just use the target as-is (the user already built the combined subject) | ||
| tracing::info!(" Time offset: {} (events already combined)", t); | ||
| target |
There was a problem hiding this comment.
time_offset in BestDosePosterior::optimize is effectively ignored (it only logs and returns target unchanged). This is inconsistent with the parameter docs (“past/future concatenation”) and makes the API misleading. Either implement the same concatenation behavior as the legacy BestDoseProblem::new path (using stored past doses) or remove time_offset from the public API.
| // The time_offset mode concatenates a dummy empty-past with the target. | |
| let final_target = match time_offset { | |
| None => target, | |
| Some(t) => { | |
| // When using time_offset without past data in the target itself, | |
| // we just use the target as-is (the user already built the combined subject) | |
| tracing::info!(" Time offset: {} (events already combined)", t); | |
| target | |
| // In this optimize() API, callers must provide a fully combined target subject | |
| // (i.e., any past/future concatenation must already be reflected in `target`). | |
| let final_target = match time_offset { | |
| None => target, | |
| Some(t) => { | |
| // For this API, using `time_offset` is not supported: it would be misleading | |
| // to accept it but ignore it. Callers should build any past/future | |
| // concatenation into `target` themselves (e.g., via helper utilities). | |
| tracing::warn!( | |
| "time_offset = {} was provided to BestDosePosterior::optimize, \ | |
| but time_offset-based concatenation is not supported in this API. \ | |
| Please build the combined subject into `target` instead.", | |
| t | |
| ); | |
| return Err(anyhow::anyhow!( | |
| "BestDosePosterior::optimize does not support `time_offset`. \ | |
| Build the past/future concatenated subject into `target` instead." | |
| )); |
time_offset now represents a gap after the last past event, not an absolute time. With past data ending at t=18 and time_offset=0, the future starts at t=18 (no gap). The effective absolute offset is computed internally as max_past_time + time_offset.
|
Bencher Report| Branch | feat/new-bestdose-api |
| Testbed | mhovd-pgx |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| bimodal_ke_npag | 📈 view plot 🚷 view threshold | 4,891.80 ms(+0.08%)Baseline: 4,887.93 ms | 5,166.30 ms (94.69%) |
| bimodal_ke_npod | 📈 view plot 🚷 view threshold | 1,377.60 ms(-5.00%)Baseline: 1,450.15 ms | 1,877.60 ms (73.37%) |
| bimodal_ke_postprob | 📈 view plot 🚷 view threshold | 453.21 ms(+7.54%)Baseline: 421.44 ms | 584.98 ms (77.47%) |
No description provided.