-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Update representation of Predictions #160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fully removes use of ndarray
There was a problem hiding this 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 migrates the codebase from the ndarray library to the faer linear algebra library for matrix operations. The change introduces a custom PredictionMatrix struct to handle 2D prediction data, replacing ndarray::Array2 usage throughout the codebase.
- Replaced
ndarraydependency withfaerfor matrix operations - Introduced
PredictionMatrixas a custom 2D container for predictions - Updated function signatures to use
faer::Mat<f64>andVec<f64>instead ofndarraytypes
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Removed ndarray dependency, added faer |
| src/simulator/likelihood/mod.rs | Introduced PredictionMatrix, updated psi() to use faer::Mat, added serialization support |
| src/simulator/equation/sde/mod.rs | Updated Predictions trait implementation to use PredictionMatrix |
| src/optimize/spp.rs | Changed from Array1<f64> to Vec<f64> and faer::Mat |
| src/lib.rs | Updated public exports to expose PredictionMatrix instead of PopulationPredictions |
| src/error/mod.rs | Removed NdarrayShapeError variant |
| examples/likelihood.rs | Migrated from ndarray::Array1 to native Vec operations |
| examples/gendata.rs | Updated to use Option-returning row() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn append_column(&mut self, column: Vec<Prediction>) -> Result<(), PharmsolError> { | ||
| if column.len() != self.nrows { | ||
| return Err(PharmsolError::OtherError(format!( | ||
| "Column length {} does not match matrix rows {}", | ||
| column.len(), | ||
| self.nrows | ||
| ))); | ||
| } | ||
| for (row, item) in self.data.iter_mut().zip(column.into_iter()) { | ||
| row.push(item); | ||
| } | ||
| self.ncols += 1; | ||
| Ok(()) | ||
| } |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The append_column method doesn't handle the empty matrix edge case correctly. When nrows == 0, the validation at line 188 will pass (since column.len() == 0), but the loop at line 195-197 won't execute (since self.data is empty), yet self.ncols will still be incremented at line 198. This creates an inconsistent state where ncols > 0 but data is empty. Add a guard: if self.nrows == 0 { return Ok(()); }
| / self.nrows() as f64; | ||
|
|
||
| let mut prediction = column.first().unwrap().clone(); | ||
| let mut prediction = (*column.first().unwrap()).clone(); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential panic if column is empty. This can occur when self.nrows() == 0. Add a guard to check if the column is empty before calling first().unwrap(), or return early if self.nrows() == 0 in the get_predictions method.
|
| Branch | faer |
| Testbed | rust-moan |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| Conditional dose modification | 📈 view plot 🚷 view threshold | 1,518.80 ns(+0.89%)Baseline: 1,505.46 ns | 1,628.13 ns (93.28%) |
| Create large dataset (100 subjects) | 📈 view plot 🚷 view threshold | 69,492.00 ns(+0.39%)Baseline: 69,224.71 ns | 75,152.87 ns (92.47%) |
| Data expand complex (1h intervals) | 📈 view plot 🚷 view threshold | 39,853.00 ns(+0.66%)Baseline: 39,591.86 ns | 40,679.21 ns (97.97%) |
| Data expand simple (1h intervals) | 📈 view plot 🚷 view threshold | 695.60 ns(-0.27%)Baseline: 697.50 ns | 713.32 ns (97.52%) |
| Data expand with additional time | 📈 view plot 🚷 view threshold | 53,363.00 ns(-2.11%)Baseline: 54,515.86 ns | 56,850.20 ns (93.87%) |
| Filter exclude subjects | 📈 view plot 🚷 view threshold | 35,988.00 ns(-3.94%)Baseline: 37,466.00 ns | 39,488.17 ns (91.14%) |
| Filter include subjects | 📈 view plot 🚷 view threshold | 10,260.00 ns(-2.21%)Baseline: 10,492.14 ns | 11,203.85 ns (91.58%) |
| Modify all bolus doses | 📈 view plot 🚷 view threshold | 1,453.90 ns(+2.41%)Baseline: 1,419.71 ns | 1,512.02 ns (96.16%) |
| Modify all infusion doses | 📈 view plot 🚷 view threshold | 1,510.10 ns(+0.42%)Baseline: 1,503.83 ns | 1,625.69 ns (92.89%) |
| SubjectBuilder multi-occasion | 📈 view plot 🚷 view threshold | 345.05 ns(+0.97%)Baseline: 341.75 ns | 371.40 ns (92.90%) |
| SubjectBuilder simple | 📈 view plot 🚷 view threshold | 96.46 ns(+2.21%)Baseline: 94.38 ns | 118.50 ns (81.40%) |
| SubjectBuilder with covariates | 📈 view plot 🚷 view threshold | 433.06 ns(+1.14%)Baseline: 428.16 ns | 445.34 ns (97.24%) |
| one_compartment | 📈 view plot 🚷 view threshold | 47,651.00 ns(+0.34%)Baseline: 47,491.71 ns | 47,965.16 ns (99.35%) |
| one_compartment_covariates | 📈 view plot 🚷 view threshold | 53,426.00 ns(+0.81%)Baseline: 52,998.14 ns | 53,648.96 ns (99.58%) |
| readme 20 | 📈 view plot 🚷 view threshold | 727,570.00 ns(+0.61%)Baseline: 723,168.57 ns | 729,306.41 ns (99.76%) |
| two_compartment | 📈 view plot 🚷 view threshold | 48,887.00 ns(+0.97%)Baseline: 48,416.29 ns | 49,333.04 ns (99.10%) |
Also removes
ndarrayfrom our dependencies