-
Notifications
You must be signed in to change notification settings - Fork 1
feat(equation): add type-state builder pattern with optional defaults #173
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
Implement type-state builder pattern for ODE, Analytical, and SDE equations that enforces required fields at compile time while making lag, fa, and init optional with sensible defaults. Changes: - Add Missing/Provided marker types for compile-time field validation - Add ODEBuilder, AnalyticalBuilder, and SDEBuilder with type-state pattern - Convert Neqs from tuple alias to struct with From<(usize, usize)> impl - Make lag default to no lag (empty HashMap) - Make fa default to 100% bioavailability (empty HashMap) - Make init default to zero initial state (no-op) - Maintain backward compatibility with existing ::new() constructors - Update all examples to use simplified builder API - Add comprehensive API comparison tests Required fields (compile-time enforced): - ODE: diffeq, out, nstates, nouteqs - Analytical: eq, seq_eq, out, nstates, nouteqs - SDE: drift, diffusion, out, nstates, nouteqs, nparticles
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 implements a type-state builder pattern for ODE, Analytical, and SDE equation types to improve API ergonomics while maintaining backward compatibility. The builder pattern enforces required fields at compile time using marker types (Missing/Provided) and provides sensible defaults for optional fields (lag, fa, init).
Key changes:
- Convert Neqs from a type alias to a struct with bidirectional tuple conversion for backward compatibility
- Add type-state builders (ODEBuilder, AnalyticalBuilder, SDEBuilder) that enforce required fields at compile time
- Provide default implementations for optional fields: empty HashMap for lag/fa, no-op for init
- Update all examples and tests to demonstrate the new builder API
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/simulator/mod.rs | Converts Neqs to a struct with From traits for tuple conversion |
| src/simulator/equation/mod.rs | Defines Missing/Provided marker types for type-state pattern |
| src/simulator/equation/ode/mod.rs | Implements ODEBuilder with compile-time field validation |
| src/simulator/equation/analytical/mod.rs | Implements AnalyticalBuilder with compile-time field validation |
| src/simulator/equation/sde/mod.rs | Implements SDEBuilder with compile-time field validation |
| src/lib.rs | Exports new builder types and marker types in public API |
| tests/api_comparison.rs | Comprehensive tests comparing old tuple API with new builder API |
| tests/numerical_stability.rs | Updates existing tests to use new builder API |
| examples/*.rs | Updates all examples to demonstrate new builder pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn neqs( | ||
| self, | ||
| neqs: impl Into<Neqs>, | ||
| ) -> ODEBuilder<DiffEqState, OutState, Provided, Provided> { |
Copilot
AI
Nov 26, 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 neqs() method only works when both NStatesState and NOuteqsState are Missing. If a user calls .nstates(1) first and then tries to call .neqs((1, 1)), it will fail to compile because NStatesState would be Provided. Consider adding documentation to clarify that neqs() is an alternative to calling nstates() and nouteqs() separately, and they cannot be mixed. Alternatively, consider if it would be more ergonomic to allow neqs() to override previously set values.
| pub fn neqs( | ||
| self, | ||
| neqs: impl Into<Neqs>, | ||
| ) -> AnalyticalBuilder<EqState, SeqEqState, OutState, Provided, Provided> { |
Copilot
AI
Nov 26, 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 neqs() method only works when both NStatesState and NOuteqsState are Missing. If a user calls .nstates(2) first and then tries to call .neqs((2, 1)), it will fail to compile because NStatesState would be Provided. Consider adding documentation to clarify that neqs() is an alternative to calling nstates() and nouteqs() separately, and they cannot be mixed. Alternatively, consider if it would be more ergonomic to allow neqs() to override previously set values.
| pub fn neqs( | ||
| self, | ||
| neqs: impl Into<Neqs>, | ||
| ) -> SDEBuilder<DriftState, DiffusionState, OutState, Provided, Provided, NParticlesState> { |
Copilot
AI
Nov 26, 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 neqs() method only works when both NStatesState and NOuteqsState are Missing. If a user calls .nstates(2) first and then tries to call .neqs((2, 1)), it will fail to compile because NStatesState would be Provided. Consider adding documentation to clarify that neqs() is an alternative to calling nstates() and nouteqs() separately, and they cannot be mixed. Alternatively, consider if it would be more ergonomic to allow neqs() to override previously set values.
|
| Branch | type-state-eqn |
| 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,908.60 ns(+15.37%)Baseline: 1,654.39 ns | 2,204.26 ns (86.59%) |
| Create large dataset (100 subjects) | 📈 view plot 🚷 view threshold | 73,674.00 ns(+7.16%)Baseline: 68,748.73 ns | 74,991.39 ns (98.24%) |
| Data expand complex (1h intervals) | 📈 view plot 🚷 view threshold | 38,527.00 ns(-2.11%)Baseline: 39,357.73 ns | 40,759.15 ns (94.52%) |
| Data expand simple (1h intervals) | 📈 view plot 🚷 view threshold | 696.88 ns(-0.66%)Baseline: 701.50 ns | 738.14 ns (94.41%) |
| Data expand with additional time | 📈 view plot 🚷 view threshold | 54,967.00 ns(+0.16%)Baseline: 54,876.67 ns | 56,403.42 ns (97.45%) |
| Filter exclude subjects | 📈 view plot 🚷 view threshold | 38,384.00 ns(+0.35%)Baseline: 38,251.60 ns | 39,498.47 ns (97.18%) |
| Filter include subjects | 📈 view plot 🚷 view threshold | 10,372.00 ns(-0.87%)Baseline: 10,462.93 ns | 10,902.58 ns (95.13%) |
| Modify all bolus doses | 📈 view plot 🚷 view threshold | 1,852.90 ns(+18.72%)Baseline: 1,560.68 ns | 2,093.92 ns (88.49%) |
| Modify all infusion doses | 📈 view plot 🚷 view threshold | 1,934.10 ns(+17.67%)Baseline: 1,643.70 ns | 2,185.45 ns (88.50%) |
| SubjectBuilder multi-occasion | 📈 view plot 🚷 view threshold | 400.30 ns(+10.76%)Baseline: 361.40 ns | 431.78 ns (92.71%) |
| SubjectBuilder simple | 📈 view plot 🚷 view threshold | 122.02 ns(+17.29%)Baseline: 104.03 ns | 140.70 ns (86.72%) |
| SubjectBuilder with covariates | 📈 view plot 🚷 view threshold | 457.65 ns(+4.38%)Baseline: 438.47 ns | 478.34 ns (95.67%) |
| one_compartment | 📈 view plot 🚷 view threshold | 36,463.00 ns(-16.76%)Baseline: 43,802.60 ns | 57,247.13 ns (63.69%) |
| one_compartment_covariates | 📈 view plot 🚷 view threshold | 43,212.00 ns(-13.08%)Baseline: 49,715.47 ns | 61,847.73 ns (69.87%) |
| readme 20 | 📈 view plot 🚷 view threshold | 543,560.00 ns(-18.02%)Baseline: 663,039.33 ns | 884,294.41 ns (61.47%) |
| two_compartment | 📈 view plot 🚷 view threshold | 36,686.00 ns(-17.94%)Baseline: 44,704.67 ns | 58,353.31 ns (62.87%) |
Implement type-state builder pattern for ODE, Analytical, and SDE equations that enforces required fields at compile time while making lag, fa, and init optional with sensible defaults.
Changes:
Required fields (compile-time enforced):