refactor: Remove likelihood calculation - #291
Open
Siel wants to merge 18 commits into
Open
Conversation
Contributor
Bencher Report
mhovd
reviewed
Jul 15, 2026
Comment on lines
+97
to
+104
|
|
||
| /// Calculate the log-likelihood of the predictions given an error model. | ||
| /// | ||
| /// This is numerically more stable than computing the likelihood and taking its log, | ||
| /// especially for extreme values or many observations. | ||
| /// | ||
| /// # Parameters | ||
| /// - `error_models`: The error models for computing observation variance | ||
| /// | ||
| /// # Returns | ||
| /// The sum of log-likelihoods for all predictions | ||
| fn log_likelihood(&self, error_models: &AssayErrorModels) -> Result<f64, PharmsolError>; | ||
| /// Visit each effective prediction without requiring callers to own a `Vec`. | ||
| fn for_each_prediction(&self, mut f: impl FnMut(&Prediction)) { | ||
| let predictions = self.get_predictions(); | ||
| for prediction in &predictions { | ||
| f(prediction); | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Is this preferred over implementing a mutable iterator?
Member
Author
There was a problem hiding this comment.
I prefer keeping for_each_prediction on the generic trait. SubjectPredictions can borrow stored points, but the SDE Array2<Prediction> implementation synthesizes effective mean predictions, so a generic mutable iterator cannot represent both consistently. A concrete mutable accessor on SubjectPredictions can be added later if a real mutation use case appears.
Comment on lines
+21
to
+22
| //! metadata, and NCA. Estimation crates own scoring, objectives, priors, | ||
| //! algorithms, diagnostics, covariance, and fit semantics. |
Collaborator
There was a problem hiding this comment.
Suggested change
| //! metadata, and NCA. Estimation crates own scoring, objectives, priors, | |
| //! algorithms, diagnostics, covariance, and fit semantics. | |
| //! metadata, and NCA. |
Collaborator
There was a problem hiding this comment.
It seems like the documentation is leaking prompt details
Member
Author
There was a problem hiding this comment.
Agreed with simplifying this. I removed the exhaustive ownership list and replaced it with a concise scope statement: pharmsol provides model execution and prediction generation, while scoring and estimation are handled downstream.
Comment on lines
+9
to
+33
|
|
||
| ## [0.28.1](https://github.com/LAPKB/pharmsol/compare/pharmsol-v0.28.0...pharmsol-v0.28.1) - 2026-07-13 | ||
| ### Added | ||
|
|
||
| ### Fixed | ||
| - Add a caller-controlled, simulation-neutral SDE particle session that pauses | ||
| at observation boundaries and can resume with retained or replaced states. | ||
| - Add focused Criterion coverage for standard SDE prediction and particle-session | ||
| retain/select paths. | ||
|
|
||
| ### Removed | ||
|
|
||
| - Remove equation-level and prediction-container observation evaluation APIs; | ||
| downstream fitting crates now generate predictions with pharmsol and evaluate | ||
| observations themselves. | ||
| - Remove residual-distribution declarations, estimator caches, and parameter | ||
| optimization helpers from pharmsol's public API. | ||
|
|
||
| ### Changed | ||
|
|
||
| - Fix data expansion ([#282](https://github.com/LAPKB/pharmsol/pull/282)) | ||
| - Relocate the existing data-only `ErrorPoly` DTO while preserving its public | ||
| path and unchanged C0-C3 transport through observations and predictions. | ||
| As before, incomplete C0-C3 rows do not create an `ErrorPoly` value. | ||
| - Keep analytical, ODE, and SDE execution focused exclusively on simulation and | ||
| prediction generation, including simulation-only examples and benchmarks. | ||
| - Run the source-scoped simulation ownership check in CI, including untracked | ||
| source files during local use. |
Collaborator
There was a problem hiding this comment.
PRs should not modify CHANGELOG directly
mhovd
requested changes
Jul 15, 2026
Siel
force-pushed
the
feature/likelihood-extraction
branch
from
July 21, 2026 21:05
0b1ab2d to
027fd4d
Compare
Collaborator
|
…traction-v2 # Conflicts: # src/dsl/native.rs # src/simulator/equation/metadata.rs # src/simulator/equation/mod.rs
Massive refactor
mhovd
reviewed
Aug 10, 2026
| #[serde(default, deserialize_with = "deserialize_option_censor")] | ||
| cens: Option<Censor>, | ||
| /// First element of the error polynomial | ||
| /// C0 observation coefficient |
Collaborator
There was a problem hiding this comment.
Its better to use polynomial coefficient, i.e. what it is
mhovd
reviewed
Aug 10, 2026
|
|
||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| /// Four floating-point coefficients attached to an observation. |
Collaborator
There was a problem hiding this comment.
Error polynomial coefficients, no need to specify float
mhovd
reviewed
Aug 10, 2026
| self.c2 | ||
| } | ||
|
|
||
| /// Get C3. |
Collaborator
There was a problem hiding this comment.
I dont think we need accessors for each coefficient, a tuple of all 4 should be good
mhovd
reviewed
Aug 10, 2026
| time: f64, | ||
| value: Option<f64>, | ||
| outeq: OutputLabel, | ||
| #[serde(default, alias = "coefficients")] |
mhovd
reviewed
Aug 10, 2026
| //! - [`Covariate`] and [`Covariates`] for time-varying subject characteristics. | ||
| //! - [`ErrorModel`], [`ResidualErrorModel`], and [`ObservationError`] for the | ||
| //! different error surfaces used by downstream workflows. | ||
| //! - [`ErrorPoly`] for the transport-neutral Pmetrics C0-C3 values attached to |
Collaborator
There was a problem hiding this comment.
Transport-neutral sounds funny
mhovd
reviewed
Aug 10, 2026
| @@ -38,12 +38,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Added | |||
|
|
|||
Collaborator
There was a problem hiding this comment.
Changelog is auto generated
* refactor: Update prediction structure * feat: Refactor Prediction structures
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
3 participants
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.
No description provided.