Factor linear/Lotka-Voltera ODE analyses through PolynomialSystem
#1007
Conversation
| model: &DiscreteDblModel, | ||
| data: LinearODEProblemData, | ||
| ) -> ODEAnalysis<LinearODESystem> { | ||
| ) -> ODEAnalysis<NumericalPolynomialSystem<u8>> { |
There was a problem hiding this comment.
The exponent for linear systems is obviously always 1. So it's tempting to use or define a singleton type, but that might be overkill. Not sure how easy or conventional it is in Rust.
There was a problem hiding this comment.
I'm not sure this accounts for constants (zero exponents) though. We'd need a type with 0 and 1
| // XXX: Lots of boilerplate to delegate the module structure of `Polynomial` to | ||
| // `Combination` because these traits cannot be derived automatically. | ||
|
|
||
| impl<Var, Coef, Exp> Sum for Polynomial<Var, Coef, Exp> |
There was a problem hiding this comment.
There's nothing here that's actually specific to Polynomial. The exact same code could be used for any AdditiveMonoid. But I don't think Rust allows instances with that sort of flexibility. We could define some reusable helper, but probably not worth it.
Note that Polynomial is an AdditiveMonoid precisely when its coefficient type is, due to the instances:
impl<Var, Coef, Exp> Add for Polynomial<Var, Coef, Exp>impl<Var, Coef, Exp> Zero for Polynomial<Var, Coef, Exp>
There was a problem hiding this comment.
Oh, and also, looking at the comment above this, maybe we should actually move the main Impl code to Combination, and just delegate to that here?
| where | ||
| Var: Ord, | ||
| Coef: Add<Output = Coef> + Zero, | ||
| Coef: Zero, |
There was a problem hiding this comment.
This slight generalisation was useful during debugging. I'm not sure if it was necessary in the end, but there's no harm.
Coming from Haskell, I am a bit surprised that the compiler doesn't warn about redundant constraints.
There was a problem hiding this comment.
Yeah, I've found myself wishing for such warnings but AFAIK, the Rust compiler doesn't warn about either redundant constraints or unnecessary constraints.
There was a problem hiding this comment.
Thanks for making a start on this refactor! I'd suggest the following alternative design:
- remove entirely the modules
catlog::simulate::ode::[linear_ode | lotka_volterra] - refactor the modules
catlog::stdlib::analyses::ode::[linear_ode | lotka_volterra]to first generate a polynomial system with symbolic coefficients and then from that a numerical polynomial system
The reason to do it this way is that it's useful to have a symbolic representation of the abstract system, before any specific numerical values are chosen. E.g., we will display that abstract system to the user as equations in LaTeX math.
For an example of how to this, see the mass_action module.
| components: interaction_coeffs | ||
| .row_iter() | ||
| .enumerate() | ||
| .zip(growth_rates.into_iter()) |
There was a problem hiding this comment.
To avoid triggering the linter in CI, you can run cargo clippy locally before pushing.
3 participants
Closes #965.
Developed alongside @patrickaldis.