Conversation
This commit adds two related features: ## NormalizedRow API (parser/) - New struct for format-agnostic data parsing - Decouples column mapping from event creation logic - Full ADDL/II expansion support (both positive and negative directions) - Refactors pmetrics.rs to use NormalizedRow internally - Enables external tools (like vial) to reuse parsing logic without reimplementing ADDL expansion ## ResidualErrorModel (data/) - New for parametric algorithms (SAEM, FOCE) - Uses prediction-based sigma (vs observation-based in ErrorModel) - Adds and functions - Documentation clarifying ErrorModel vs ResidualErrorModel usage Both features are independent but included together to avoid merge conflicts.
* nca * wip: current version * feat: nca * clenup * chore: documentation * chore: cleanup * chore: cleanup * chore: deprecating ErrorModel in favor of AssayErrorModel, subdividing the likelihood module and deprecating linear space likelihood calculation functions * feat: the Data parsing is centraliced to NormalizedRow * feat: the ErrorModel -> AssayErrorModel * feat: validation * chore: cleanup * chore: cleanup
Co-authored-by: Markus Hovd <markushh@uio.no>
… require extra size
* chore: Rename modules and structures * Update src/error/mod.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/data/parser/mod.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Name changes --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This is a major feature addition introducing Non-Compartmental Analysis (NCA) capabilities to pharmsol. The PR adds comprehensive NCA functionality for calculating standard pharmacokinetic parameters from concentration-time data, alongside refactoring error models into observation-based (assay) and prediction-based (residual) variants, and introducing a JSON model definition system with code generation.
Changes:
- Adds complete NCA module with support for λz estimation, AUC calculations, clearance parameters, and steady-state analysis
- Refactors error models into
AssayErrorModels(observation-based, for NPAG/NPOD) andResidualErrorModels(prediction-based, for SAEM/FOCE) - Introduces JSON model definition system with validation and code generation for analytical, ODE, and SDE models
- Adds observation data processing utilities and dose introspection methods
Reviewed changes
Copilot reviewed 76 out of 83 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ode_optimizations.rs | Updates likelihood calculations to use log-space and new AssayErrorModels |
| tests/nca/*.rs | New NCA integration tests covering terminal phase, quality metrics, parameters, and AUC |
| src/simulator/mod.rs | Updates documentation to use inline code formatting for macro references |
| src/simulator/likelihood/*.rs | New modules for subject-level predictions and log-likelihood calculations |
| src/simulator/equation/*.rs | Updates to use AssayErrorModels and log-likelihood calculations |
| src/optimize/*.rs | Updates to use new log-likelihood matrix API |
| src/nca/*.rs | New NCA module with analysis, types, traits, and utilities |
| src/lib.rs | Updates public API exports to include NCA and refactored error models |
| src/json/*.rs | New JSON model definition system with validation and code generation |
| src/data/*.rs | Adds observation processing, dose introspection, and data row parsing |
| src/error/mod.rs | Updates error types to use DataError instead of PmetricsError |
| examples/*.rs | New NCA and JSON compilation examples, updates to existing examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub use bioavailability::{ | ||
| bioavailability, bioequivalence, compare, metabolite_parent_ratio, BioavailabilityResult, | ||
| BioequivalenceResult, |
There was a problem hiding this comment.
The public API exports bioavailability functions but there are no corresponding module definitions or implementations visible in the diff. This suggests missing module files or incomplete implementation that could cause compilation errors.
| "output": "x[0] / V", | ||
| "neqs": [1, 1], |
There was a problem hiding this comment.
The output equation x[0] / V references state variable index 0, but for a one-compartment IV model this should reference the concentration in the central compartment. Verify that this indexing is correct for the analytical function being used.
| .value() | ||
| .map_or_else(|| ".".to_string(), |v| v.to_string()); | ||
| let outeq = (obs.outeq() + 1).to_string(); | ||
| let outeq = obs.outeq().to_string(); |
There was a problem hiding this comment.
The outeq is now written directly without adding 1, changing from 1-indexed to 0-indexed output. This is a breaking change in the file format that should be documented in a migration guide or changelog to help users update their data files.
| let outeq = obs.outeq().to_string(); | |
| let outeq = (obs.outeq() + 1).to_string(); |
| ### Other | ||
|
|
||
| - *(Exa)* when installing Vial on MacOs, the environment varaibles are not completly shared to the sandbox in which Vial is running, this changes are meant to provide vial a better way to approach finding the rust binary ([#181](https://github.com/LAPKB/pharmsol/pull/181)) | ||
| - _(Exa)_ when installing Papir on MacOs, the environment varaibles are not completly shared to the sandbox in which Papir is running, this changes are meant to provide papir a better way to approach finding the rust binary ([#181](https://github.com/LAPKB/pharmsol/pull/181)) |
There was a problem hiding this comment.
Corrected spelling of 'varaibles' to 'variables' and 'completly' to 'completely'.
| - _(Exa)_ when installing Papir on MacOs, the environment varaibles are not completly shared to the sandbox in which Papir is running, this changes are meant to provide papir a better way to approach finding the rust binary ([#181](https://github.com/LAPKB/pharmsol/pull/181)) | |
| - _(Exa)_ when installing Papir on MacOs, the environment variables are not completely shared to the sandbox in which Papir is running, this changes are meant to provide papir a better way to approach finding the rust binary ([#181](https://github.com/LAPKB/pharmsol/pull/181)) |
| let results = subject.nca(&NCAOptions::default(), 0); | ||
| let result = results[0].as_ref().expect("NCA failed"); |
There was a problem hiding this comment.
The example shows calling .nca() with two arguments, but according to the trait definition in src/nca/traits.rs, the method signature is .nca(&self, options: &NCAOptions) -> Result<NCAResult, NCAError> which only takes one argument. This example code will not compile.
| let results = subject.nca(&NCAOptions::default(), 0); | |
| let result = results[0].as_ref().expect("NCA failed"); | |
| let result = subject.nca(&NCAOptions::default()).expect("NCA failed"); |
| }; | ||
| write!( | ||
| f, | ||
| "Time: {:.2}\tObs: {:.4}\tPred: {:.4}\tOuteq: {:.2}", |
There was a problem hiding this comment.
The format specifier for outeq is {:.2} which is for floating point numbers, but outeq is a usize. This should use {} instead for integer formatting.
| "Time: {:.2}\tObs: {:.4}\tPred: {:.4}\tOuteq: {:.2}", | |
| "Time: {:.2}\tObs: {:.4}\tPred: {:.4}\tOuteq: {}", |
|
| Branch | feat/new-nca |
| Testbed | mhovd-pgx |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| Analytical vs ODE/One-compartment IV/Analytical | 📈 view plot 🚷 view threshold | 837.66 ns(-2.10%)Baseline: 855.62 ns | 981.77 ns (85.32%) |
| Analytical vs ODE/One-compartment IV/ODE | 📈 view plot 🚷 view threshold | 17,553.00 ns(-0.69%)Baseline: 17,675.00 ns | 18,168.42 ns (96.61%) |
| Analytical vs ODE/One-compartment oral/Analytical | 📈 view plot 🚷 view threshold | 846.46 ns(-0.59%)Baseline: 851.47 ns | 1,025.00 ns (82.58%) |
| Analytical vs ODE/One-compartment oral/ODE | 📈 view plot 🚷 view threshold | 25,811.00 ns(-0.03%)Baseline: 25,817.50 ns | 26,350.92 ns (97.95%) |
| Analytical vs ODE/Two-compartment IV/Analytical | 📈 view plot 🚷 view threshold | 876.97 ns(-5.38%)Baseline: 926.82 ns | 1,097.98 ns (79.87%) |
| Analytical vs ODE/Two-compartment IV/ODE | 📈 view plot 🚷 view threshold | 26,218.00 ns(-0.16%)Baseline: 26,259.50 ns | 26,659.27 ns (98.34%) |
| Analytical vs ODE/Two-compartment oral/Analytical | 📈 view plot 🚷 view threshold | 983.02 ns(-0.48%)Baseline: 987.80 ns | 1,162.04 ns (84.59%) |
| Analytical vs ODE/Two-compartment oral/ODE | 📈 view plot 🚷 view threshold | 29,373.00 ns(+0.04%)Baseline: 29,360.00 ns | 30,352.71 ns (96.77%) |
| Conditional dose modification | 📈 view plot 🚷 view threshold | 1,194.80 ns(-0.04%)Baseline: 1,195.30 ns | 1,218.58 ns (98.05%) |
| Create large dataset (100 subjects) | 📈 view plot 🚷 view threshold | 56,271.00 ns(+3.42%)Baseline: 54,410.00 ns | 59,323.17 ns (94.86%) |
| Data expand complex (1h intervals) | 📈 view plot 🚷 view threshold | 27,160.00 ns(-0.85%)Baseline: 27,391.50 ns | 29,191.42 ns (93.04%) |
| Data expand simple (1h intervals) | 📈 view plot 🚷 view threshold | 484.58 ns(-0.37%)Baseline: 486.40 ns | 503.10 ns (96.32%) |
| Data expand with additional time | 📈 view plot 🚷 view threshold | 37,664.00 ns(-0.89%)Baseline: 38,003.25 ns | 40,190.82 ns (93.71%) |
| Filter exclude subjects | 📈 view plot 🚷 view threshold | 30,876.00 ns(-0.56%)Baseline: 31,049.25 ns | 31,675.99 ns (97.47%) |
| Filter include subjects | 📈 view plot 🚷 view threshold | 8,206.80 ns(+2.38%)Baseline: 8,015.70 ns | 8,661.63 ns (94.75%) |
| Modify all bolus doses | 📈 view plot 🚷 view threshold | 1,154.00 ns(-0.60%)Baseline: 1,161.00 ns | 1,180.80 ns (97.73%) |
| Modify all infusion doses | 📈 view plot 🚷 view threshold | 1,195.60 ns(-0.84%)Baseline: 1,205.70 ns | 1,258.39 ns (95.01%) |
| SubjectBuilder multi-occasion | 📈 view plot 🚷 view threshold | 259.75 ns(-0.39%)Baseline: 260.75 ns | 264.12 ns (98.35%) |
| SubjectBuilder simple | 📈 view plot 🚷 view threshold | 102.58 ns(-0.65%)Baseline: 103.25 ns | 105.40 ns (97.33%) |
| SubjectBuilder with covariates | 📈 view plot 🚷 view threshold | 250.91 ns(-5.57%)Baseline: 265.70 ns | 304.55 ns (82.39%) |
| nca_auc_cmax_metrics | 📈 view plot 🚷 view threshold | 587.16 ns | |
| nca_lambda_z_candidates | 📈 view plot 🚷 view threshold | 628.59 ns | |
| nca_population/10 | 📈 view plot 🚷 view threshold | 46,348.00 ns | |
| nca_population/100 | 📈 view plot 🚷 view threshold | 143,860.00 ns | |
| nca_population/500 | 📈 view plot 🚷 view threshold | 318,660.00 ns | |
| nca_single_subject | 📈 view plot 🚷 view threshold | 995.94 ns | |
| one_compartment | 📈 view plot 🚷 view threshold | 19,436.00 ns(-1.04%)Baseline: 19,640.00 ns | 20,373.66 ns (95.40%) |
| one_compartment_covariates | 📈 view plot 🚷 view threshold | 26,793.00 ns(+1.29%)Baseline: 26,452.25 ns | 28,856.21 ns (92.85%) |
| readme 20 | 📈 view plot 🚷 view threshold | 303,060.00 ns(-0.64%)Baseline: 305,007.50 ns | 324,290.40 ns (93.45%) |
| two_compartment | 📈 view plot 🚷 view threshold | 21,615.00 ns(-0.77%)Baseline: 21,782.50 ns | 22,384.73 ns (96.56%) |
| @@ -0,0 +1,792 @@ | |||
| { | |||
There was a problem hiding this comment.
I feel like we shold be able to define these schemas in Rust using a struc, which is converted to this scheme. There has to be some way of doing that, at least I hope.
There was a problem hiding this comment.
The JSON itself can be defined on any language, but the Schema itself (to my knowledge) is always defined in JSON
| //! # Design | ||
| //! | ||
| //! All functions in this module are **pure math** — no dependency on data structures, | ||
| //! no BLQ filtering, no error types beyond what the caller can check. They accept | ||
| //! raw slices and an [`AUCMethod`], and return `f64`. |
There was a problem hiding this comment.
This documentation is a little out of place, and seems very LLM-y
| /// Linear trapezoidal AUC for a single segment | ||
| #[inline] | ||
| fn auc_linear(c1: f64, c2: f64, dt: f64) -> f64 { | ||
| (c1 + c2) / 2.0 * dt | ||
| } | ||
|
|
||
| /// Log-linear AUC for a single segment (assumes c1 > c2 > 0) | ||
| #[inline] | ||
| fn auc_log(c1: f64, c2: f64, dt: f64) -> f64 { | ||
| (c1 - c2) * dt / (c1 / c2).ln() | ||
| } |
There was a problem hiding this comment.
Maybe it will be apparent as I dig through the code, but I think it would make more sense to use the times directly instead of the dt. But I may change my mind as I go through the PR.
| /// calculation cannot know Tmax context. Use [`auc`] or | ||
| /// [`auc_segment_with_tmax`] for proper LinLog handling. | ||
| #[inline] | ||
| pub fn auc_segment(t1: f64, c1: f64, t2: f64, c2: f64, method: &AUCMethod) -> f64 { |
There was a problem hiding this comment.
This implements Copy, so I don't think it has to be a reference?
There was a problem hiding this comment.
Even if the struct implements Copy, passing it by reference is cheaper
| let dt = t2 - t1; | ||
| if dt <= 0.0 { | ||
| return 0.0; | ||
| } |
There was a problem hiding this comment.
Instead of silently returning zero, perhaps return an error?
| /// Add an extravascular bolus dose (oral, SC, IM, etc.) | ||
| /// | ||
| /// Convenience alias for `.bolus(time, amount, 0)` — targets the depot compartment. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `time` - Time of the bolus dose | ||
| /// * `amount` - Amount of drug administered | ||
| pub fn bolus_ev(self, time: f64, amount: f64) -> Self { | ||
| self.bolus(time, amount, 0) | ||
| } | ||
|
|
||
| /// Add an intravenous bolus dose | ||
| /// | ||
| /// Convenience alias for `.bolus(time, amount, 1)` — targets the central compartment. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `time` - Time of the bolus dose | ||
| /// * `amount` - Amount of drug administered | ||
| pub fn bolus_iv(self, time: f64, amount: f64) -> Self { | ||
| self.bolus(time, amount, 1) | ||
| } | ||
|
|
||
| /// Add an intravenous infusion | ||
| /// | ||
| /// Convenience alias for `.infusion(time, amount, 1, duration)` — targets the central compartment. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `time` - Start time of the infusion | ||
| /// * `amount` - Total amount of drug to be administered | ||
| /// * `duration` - Duration of the infusion in time units | ||
| pub fn infusion_iv(self, time: f64, amount: f64, duration: f64) -> Self { | ||
| self.infusion(time, amount, 1, duration) | ||
| } |
There was a problem hiding this comment.
I don't see the use for these - and they only seem to be used in an example. Consider removing.
A copy before this change is at #209
| .build(); | ||
|
|
||
| let options = NCAOptions::default().with_tau(12.0); // 12-hour dosing interval | ||
| let result = subject.nca(&options).expect("NCA analysis failed"); |
No description provided.