refactor: Remove likelihood calculation#291
Open
Siel wants to merge 4 commits into
Open
Conversation
Contributor
Bencher Report
mhovd
reviewed
Jul 15, 2026
| - name: Check formatting | ||
| run: cargo fmt --all -- --check | ||
|
|
||
| - name: Check simulation ownership boundary |
| } | ||
|
|
||
| /// Create an empty cache with the same capacity but no shared entries. | ||
| pub(crate) fn detached(&self) -> Self { |
Collaborator
There was a problem hiding this comment.
Can we rename to e.g. reset?
With documentation comment
/// Reset the cache
///
/// Creates a new, unique cache that is not shared with caches from potentially linked equations
| /// * `value` - Observed value (e.g., drug concentration) | ||
| /// * `outeq` - Output label corresponding to this observation | ||
| /// * `errorpoly` - Optional error polynomial coefficients (c0, c1, c2, c3) | ||
| /// * `errorpoly` - Optional C0-C3 data |
Collaborator
There was a problem hiding this comment.
I prefer the old errorpoly - Optional error polynomial coefficients (c0, c1, c2, c3)
| } | ||
|
|
||
| /// Set the [ErrorPoly] for this observation | ||
| /// Set the [`ErrorPoly`] data for this observation. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// Set the [`ErrorPoly`] data for this observation. | |
| /// Set the [`ErrorPoly`] coefficients for this observation. |
| } | ||
|
|
||
| /// Get a mutable reference to the error polynomial | ||
| /// Get a mutable reference to the optional [`ErrorPoly`] data. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// Get a mutable reference to the optional [`ErrorPoly`] data. | |
| /// Get a mutable reference to the optional [`ErrorPoly`] coefficients. |
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?
| pub(crate) prediction: f64, | ||
| pub(crate) outeq: usize, | ||
| pub(crate) errorpoly: Option<ErrorPoly>, | ||
| pub(crate) state: Vec<f64>, |
Collaborator
There was a problem hiding this comment.
Not related to this PR, but is there mapping of state index to metadata state name?
Comment on lines
+15
to
+17
| pub struct SubjectPredictions { | ||
| predictions: Vec<Prediction>, | ||
| } |
Collaborator
There was a problem hiding this comment.
I know we have had this structure for some time, but I am a little torn about it.
My biggest gripe is that it is disconnected from the data and subject from and for which it was generated.
Would it make sense to have predictions as part of the data?
Or as part of the Observation? I don't know.
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
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
|
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
2 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.