Factor linear/Lotka-Voltera ODE analyses through PolynomialSystem#1007
Factor linear/Lotka-Voltera ODE analyses through PolynomialSystem#1007georgefst wants to merge 9 commits intoToposInstitute:mainfrom
PolynomialSystem#1007Conversation
| 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
There was a problem hiding this comment.
We could of course define a zero-or-one type instead. But still, probably overkill.
| // 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.
In the last commit, we've refactored so that polynomials with symbolic coefficients are created first, and added some LaTeX tests to exercise the symbolic versions. Hopefully this is roughly what you had in mind. If so, we'll then proceed with a similar change for the Lotka-Volterra systems. |
georgefst
force-pushed
the
polynomial-lv
branch
from
7d9792d to
e7f9913
Compare
| //! The main entry point for this module is | ||
| //! [`linear_ode_analysis`](SignedCoefficientBuilder::linear_ode_analysis). |
There was a problem hiding this comment.
Ah, I suppose this is no longer true now that this module also contains the new generalised linear_polynomial_system as well.
| \end{align*} | ||
| $$ | ||
| "#]]; | ||
| expected.assert_eq(&matrix_example().to_latex_string()); |
3 participants
Closes #965.
Developed alongside @patrickaldis.