feat: Refactor outputs#178
Conversation
Also moves outputs from a monolithic file to a module
|
Bencher Report| Branch | outputs-refactor |
| Testbed | pmcore-runner |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result seconds (s) (Result Δ%) | Upper Boundary seconds (s) (Limit %) |
|---|---|---|---|
| bimodal_ke_npag | 📈 view plot 🚷 view threshold | 34.49 s(+57.80%)Baseline: 21.85 s | 48.29 s (71.41%) |
| bimodal_ke_npod | 📈 view plot 🚷 view threshold | 44.01 s(+99.98%)Baseline: 22.01 s | 68.24 s (64.50%) |
| bimodal_ke_postprob | 📈 view plot 🚷 view threshold | 15.31 s(+99.58%)Baseline: 7.67 s | 23.71 s (64.59%) |
The helper function does not need to own the data, and as such it can take a reference to the data instead.
Pred.csv now contains observations (where available), and op.csv is the filtered version of pred.csv that only includes predictions where there exists an associated observation
Also bumps pharmsol
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the output structures to improve modularity and usability by introducing dedicated wrapper structures for different data types. The changes aim to make outputs agnostic to underlying data while adding serialization/deserialization support.
- Introduces a new
Weightswrapper struct aroundfaer::Col<f64>with serialization support - Adds CSV and serde serialization/deserialization capabilities to
Theta,Psi, and newPosteriorstructures - Refactors prediction generation into a dedicated
NPPredictionsstructure with calculation logic - Reorganizes output module structure by moving cycle logging and predictions into separate submodules
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/structs/weights.rs | New wrapper struct for weights with serialization and utility methods |
| src/structs/theta.rs | Adds CSV and serde serialization/deserialization methods |
| src/structs/psi.rs | Adds CSV and serde serialization/deserialization methods |
| src/structs/mod.rs | Exports the new weights module |
| src/routines/output/predictions.rs | New dedicated structure for handling predictions with calculation logic |
| src/routines/output/posterior.rs | New structure for posterior probabilities with serialization support |
| src/routines/output/cycles.rs | Extracted cycle logging functionality into separate module |
| src/routines/output/mod.rs | Refactored to use new structures and removed inline cycle/prediction code |
| src/routines/settings.rs | Updated to use new OutputFile API |
| src/routines/logger.rs | Updated to use new OutputFile API |
| src/routines/evaluation/ipm.rs | Updated to return new Weights type |
| src/algorithms/*.rs | Updated to use new Weights type and refactored cycle structures |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /// Each row represents a support point, each column represents a parameter | ||
| /// Note: This only reads the matrix values, not the parameter metadata |
There was a problem hiding this comment.
The CSV deserialization methods in both Theta::from_csv and Psi::from_csv create empty Parameters without preserving parameter metadata. This limitation should be documented in the function's docstring to inform users that they need to set parameters separately after loading from CSV.
| /// Each row represents a support point, each column represents a parameter | |
| /// Note: This only reads the matrix values, not the parameter metadata | |
| /// Each row represents a support point, each column represents a parameter. | |
| /// | |
| /// Note: This only reads the matrix values and does not preserve or load parameter metadata. | |
| /// The returned `Theta` will have empty `Parameters`. You must set the parameters separately after loading from CSV. |
| let mat = Mat::from_fn(nrows, ncols, |i, j| rows[i][j]); | ||
|
|
||
| // Create empty parameters - user will need to set these separately | ||
| let parameters = Parameters::new(); |
There was a problem hiding this comment.
The CSV deserialization methods in both Theta::from_csv and Psi::from_csv create empty Parameters without preserving parameter metadata. This limitation should be documented in the function's docstring to inform users that they need to set parameters separately after loading from CSV.
| for (i, row) in rows.iter().enumerate() { | ||
| if row.len() != ncols { | ||
| bail!("Row {} has {} columns, expected {}", i, row.len(), ncols); | ||
| } | ||
| } |
There was a problem hiding this comment.
The matrix dimension validation logic is duplicated across Theta::from_csv, Psi::from_csv, and Posterior::from_csv. Consider extracting this into a shared utility function to reduce code duplication.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR aims to improve the usability and modularity of the outputs.
Each outputs should be a dedicated structure that is agnostic to the underlying data, while also adding support for easy serialization and deserialization.