Skip to content

Fix bestdose posterior#281

Open
R-garreau wants to merge 5 commits into
mainfrom
fix-bestdose-posterior
Open

Fix bestdose posterior#281
R-garreau wants to merge 5 commits into
mainfrom
fix-bestdose-posterior

Conversation

@R-garreau
Copy link
Copy Markdown

This pull request introduces a significant enhancement to the BestDose algorithm by adding support for configurable Stage 2 optimization strategies. It allows users to choose between the original "dual" optimization (posterior and uniform) and a new "posterior-only" strategy, improving flexibility and transparency. The changes include API extensions, a new optimization pathway, and improved output for support points. Additionally, there are bug fixes and refactoring for clarity and correctness.

Major new features and API changes:

  • Added the OptimizationStrategy enum (with Dual and PosteriorOnly variants) to control Stage 2 optimization in BestDoseProblem, with Dual as the default for backward compatibility. Methods to set and query the strategy were added to BestDoseProblem, and the API was updated to expose this option. (src/bestdose/types.rs, src/bestdose/mod.rs, examples/bestdose_posterior.rs, [[1]](https://github.com/LAPKB/PMcore/pull/281/files#diff-5579cab66369604239b0d08fb7a22468dc271026dbceefc5999acc66b616fee0R186-R212), [[2]](https://github.com/LAPKB/PMcore/pull/281/files#diff-63aea7f46152f96d93294845ba544f57e0386115a6a4ac1adaecd9795be08fd7L305-R305), [[3]](https://github.com/LAPKB/PMcore/pull/281/files#diff-63aea7f46152f96d93294845ba544f57e0386115a6a4ac1adaecd9795be08fd7R802-R807), [[4]](https://github.com/LAPKB/PMcore/pull/281/files#diff-63aea7f46152f96d93294845ba544f57e0386115a6a4ac1adaecd9795be08fd7R833-R837))

  • The optimize method of BestDoseProblem now dispatches to either the dual or posterior-only optimization path based on the selected strategy, with appropriate logging and result handling. (src/bestdose/mod.rs, [src/bestdose/mod.rsR772-R790](https://github.com/LAPKB/PMcore/pull/281/files#diff-63aea7f46152f96d93294845ba544f57e0386115a6a4ac1adaecd9795be08fd7R772-R790))

  • New example examples/bestdose_posterior.rs demonstrates the use of the posterior-only optimization strategy and prints detailed summaries of posterior support points and weights. (examples/bestdose_posterior.rs, [examples/bestdose_posterior.rsR1-R161](https://github.com/LAPKB/PMcore/pull/281/files#diff-dba6e21e48e516e56e39cd8bc1796d949ad17074b73a261ecda029b04ffe2e38R1-R161))

Algorithmic and implementation improvements:

  • Refactored the Stage 2 and Stage 3 optimization logic: introduced a shared finalize_optimization function, and added a dedicated posterior_optimization function for the posterior-only pathway. The dual optimization path now delegates to this shared logic. (src/bestdose/optimization.rs, [[1]](https://github.com/LAPKB/PMcore/pull/281/files#diff-6a560c2551efb495e2b115bb86555df9abd5ba9b0f1159ea51ff5d68391d7171R183-R254), [[2]](https://github.com/LAPKB/PMcore/pull/281/files#diff-6a560c2551efb495e2b115bb86555df9abd5ba9b0f1159ea51ff5d68391d7171L258-R330))

  • Improved the NPAGFULL11 filtering in posterior calculation to use direct Bayesian updating rather than a separate optimization step, clarifying the intent and improving transparency. (src/bestdose/posterior.rs, [[1]](https://github.com/LAPKB/PMcore/pull/281/files#diff-0ddd7fe773579ff845c545728699cd054a3823fd4bed59b7d8de18d2d08f2a35L89-R89), [[2]](https://github.com/LAPKB/PMcore/pull/281/files#diff-0ddd7fe773579ff845c545728699cd054a3823fd4bed59b7d8de18d2d08f2a35L104-R178))

Bug fixes:

  • Fixed a bug in the cost calculation where population means were incorrectly weighted by posterior weights instead of prior weights. (src/bestdose/cost.rs, [src/bestdose/cost.rsL513-R513](https://github.com/LAPKB/PMcore/pull/281/files#diff-072b6f7fdcffee448fdc9b9051fa53f7ea2d449daea95b09fd932c2f09799ddcL513-R513))

Minor changes and improvements:

  • Enhanced output in examples to print support point summaries and improved logging for optimization strategy selection. (examples/bestdose.rs, [[1]](https://github.com/LAPKB/PMcore/pull/281/files#diff-c762aa46050859e4c42d5576d0408956abd81bbbc622b9ec44e25378f31bf724R134-R166), src/bestdose/mod.rs, [[2]](https://github.com/LAPKB/PMcore/pull/281/files#diff-63aea7f46152f96d93294845ba544f57e0386115a6a4ac1adaecd9795be08fd7R713))
  • Minor code cleanup and doc updates. (src/bestdose/posterior.rs, [[1]](https://github.com/LAPKB/PMcore/pull/281/files#diff-0ddd7fe773579ff845c545728699cd054a3823fd4bed59b7d8de18d2d08f2a35L56), [[2]](https://github.com/LAPKB/PMcore/pull/281/files#diff-0ddd7fe773579ff845c545728699cd054a3823fd4bed59b7d8de18d2d08f2a35L89-R89))

Other:

  • Updated example data file references for consistency. (examples/bestdose.rs, [examples/bestdose.rsL76-R76](https://github.com/LAPKB/PMcore/pull/281/files#diff-c762aa46050859e4c42d5576d0408956abd81bbbc622b9ec44e25378f31bf724L76-R76))

These changes provide a more flexible and robust framework for dose optimization, enabling easier experimentation and clearer results.

Siel and others added 5 commits March 24, 2026 20:44
- remove burke and implement direct bayes weight update
- update bimodal_ke example
- split methods of dual optimization
- User can now chose to run posterior only or dual (posterior and uniform)
- add examples (`bestdose_posterior.rs`) to compare dual and posterior only

Co-Authored-By: Julian Otalvaro <1023006+Siel@users.noreply.github.com>
Co-Authored-By: Markus Hovd <66058642+mhovd@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 20, 2026 20:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the BestDose API and optimization pipeline to support a configurable Stage 2 strategy (dual vs posterior-only), refactors Stage 3 finalization to be shared, and updates posterior filtering to use a direct Bayes update.

Changes:

  • Added OptimizationStrategy and plumbing on BestDoseProblem to select Stage 2 optimization behavior (Dual default vs PosteriorOnly).
  • Refactored Stage 2/3 optimization code by introducing a shared finalize_optimization and a dedicated posterior-only optimization path.
  • Reworked NPAGFULL11 filtering to compute posterior weights via direct Bayesian updating, and updated examples to show support-point summaries and posterior-only usage.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/bestdose/types.rs Introduces OptimizationStrategy and stores it on BestDoseProblem.
src/bestdose/mod.rs Re-exports the new enum, adds setters/getters, and dispatches optimize() based on the selected strategy.
src/bestdose/optimization.rs Adds posterior-only Stage 2 path and factors Stage 3 result building into finalize_optimization().
src/bestdose/posterior.rs Replaces the prior burke-based step with a direct Bayes update for NPAGFULL11 filtering.
src/bestdose/cost.rs Adjusts weight usage in cost computation (but currently reintroduces a weighting bug; see comment).
examples/bestdose.rs Updates prior file path and prints support-point/weight summaries.
examples/bestdose_posterior.rs New example demonstrating posterior-only optimization and printing detailed summaries.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/bestdose/cost.rs
sumsq_i += se;
// Calculate population mean using PRIOR probabilities
y_bar[j] += prior_prob * pj;
y_bar[j] += post_prob * pj;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not an error.
the bias trade-off is calculated based on the posterior, just need to update documentation

Comment thread src/bestdose/mod.rs
Comment on lines 710 to 715
tracing::info!(" Support points: {}", posterior_theta.matrix().nrows());
tracing::info!(" Target type: {:?}", target_type);
tracing::info!(" Bias weight (λ): {}", bias_weight);
tracing::info!(" Optimization strategy: {}", OptimizationStrategy::Dual);

Ok(BestDoseProblem {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good catch

Comment thread src/bestdose/mod.rs
settings,
doserange,
bias_weight,
optimization_strategy: OptimizationStrategy::Dual,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be hardcoded?

Comment thread src/bestdose/mod.rs
Comment on lines +772 to +785
match self.optimization_strategy {
OptimizationStrategy::Dual => {
tracing::info!("╔══════════════════════════════════════════════════════════╗");
tracing::info!("║ BestDose Algorithm: STAGE 2 & 3 ║");
tracing::info!("║ Dual Optimization + Final Predictions ║");
tracing::info!("╚══════════════════════════════════════════════════════════╝");

optimization::dual_optimization(&self)
}
OptimizationStrategy::PosteriorOnly => {
tracing::info!("╔══════════════════════════════════════════════════════════╗");
tracing::info!("║ BestDose Algorithm: STAGE 2 & 3 ║");
tracing::info!("║ Posterior Optimization Only + Final Predictions ║");
tracing::info!("╚══════════════════════════════════════════════════════════╝");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While pretty, perhaps we should stick to one-line logs?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take on this is do we need it ?
While this appear in console when running the inline command, it will never appear in the app.
Plus, when logging the run to find why it crashes, it makes the log unecessary long in my opinion.

i'd rather have a count that could be use to track progress.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. And if this is intented for console, then we should not print it to logs using tracing, but use println! instead. But for a binary, I think it should be limited.

Comment thread src/bestdose/types.rs
Comment on lines +198 to +202
impl Default for OptimizationStrategy {
fn default() -> Self {
Self::Dual
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an argument can be made to have the PosteriorOnly (perhaps renamed to Posterior or more explicitly MAP, as the default.

Comment thread src/bestdose/cost.rs
@@ -510,7 +510,7 @@ pub fn calculate_cost(problem: &BestDoseProblem, candidate_doses: &[f64]) -> Res
let se = (obs_val - pj).powi(2);
sumsq_i += se;
// Calculate population mean using PRIOR probabilities
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Calculate population mean using PRIOR probabilities
// Calculate population mean using the posterior probabilities

Comment thread src/bestdose/posterior.rs
Comment on lines +113 to +115
if prior_weight <= 0.0 {
continue;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unnecessary

Comment thread src/bestdose/posterior.rs
}

let mut log_weight = prior_weight.ln();
let mut is_zero = false;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To improve code clarity, it is better to have this in the if statement itself, since it is so small.

Comment thread src/bestdose/types.rs
Comment on lines +193 to +197
pub enum OptimizationStrategy {
Dual,
PosteriorOnly,
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we perhaps "rebrand" the Dual and PosteriorOnly to be more informative?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants