Refactor ODE analyses through ODESemantics in catlog, catlog-wasm, and frontend - #1364
Refactor ODE analyses through ODESemantics in catlog, catlog-wasm, and frontend#1364tim-at-topos wants to merge 38 commits into
ODESemantics in catlog, catlog-wasm, and frontend#1364Conversation
WIP: Rethinking traits WIP: Some tests only half failing WIP: Tests passing; time to tidy ENH: Build derived model of th_signed_polynomial_ode_system in mass_action ENH: Mass-action for stock-flow; DEL: Mass-action for signed stock-flow ENH: struct for transition / flow interfaces WIP: Starting on Lotka-Volterra WIP: Failing tests FIX: Lotka-Volterra tests passing FIX: Working analysis (frontend) ENH: Lotka-Volterra equations ENH: Linear ODE refactor ENH: Linear ODE equations WIP: Starting on ODESemantics WIP: lotka_volterra_semantics() WIP: build_system_from_ode_semantics WIP: DblModelForODESemantics WIP: ODESemanticsAnalysis and ODESemanticsProblemData WIP: ODESemantics trait WIP: Documentation WIP: ODESemantics for mass-action WIP: Cleaning up types, but mass-action still frustrating WIP: Big reshuffle (moving functions out from a struct) WIP: Fixing mass-action again WIP: terrible code WIP: Changed from ObGen to Ob WIP: Stock-flow mass-action FIX: Passing catlog tests Rename LinearODE -> LCC FIX: Documentation TODO: Redesign
…n_config_form.tsx
| // feature = "serde-wasm", | ||
| // tsify(into_wasm_abi, from_wasm_abi, hashmap_as_object) | ||
| // )] | ||
| pub trait ODESemanticsProblemData<P: ODEParameterType> { |
There was a problem hiding this comment.
let's talk about this, but on the surface of things it seems to me that you should be able to do
pub trait ODESemanticsProblemData<P: ODEParameterType> {
type EquationsData: ODESemanticsEquationsData;
const EQUATIONS_DATA: Self::EquationsData;
const INITIAL_VALUES: &'static [(QualifiedName, f32)];
const DURATION: f32;
...it's not typically possible to put a hasmap into a const, but it's easy enough to construct a hashmap from slices like this using collect.
There was a problem hiding this comment.
It's possible that i'll be able to answer these questions myself with more reading, but taking notes as they jump out at me.
maybe a follow-up question is: must this be a trait?
one could imagine a struct pattern like this
pub struct ODESemanticsProblemData<E: ODESemanticsEquationsDatat, P: ODEParameterType> {
equations_data: E,
initial_values: HashMap<QualifiedName, f32>,
duration: f32,
_param_type: PhantomData<P>
}
impl<E: ODESemanticsEquationsDatat, P: ODEParameterType> ODESemanticsProblemData<E,P> {
pub fn build_analysis(....) { ... }
}and then we can figure out what extend_scalars is supposed to do?
There was a problem hiding this comment.
i'm so silly, this should definitely be a struct instead of a trait i think
There was a problem hiding this comment.
OK, I think I see the idea here. This would involve moving more things into the EquationsData than there currently are, but maybe that's not a bad idea... however, I think this would break all existing analysis documents, so we would have to finally do some sort of migration on this.
If what I've written isn't too unbearable, then it might actually be a good idea to stick with it.
For some arcane GitHub reason, #1251 was automatically closed and I cannot reopen it. Then #1363 was a clone with #1319 merged in, but the automatic rebase that GitHub did with its new stacked PR feature messed everything up and broke all the tests. This is a third-time-lucky PR. I'll repeat all the original PR descriptions below for the sake of the reader.
This PR includes a massive redesign of the way that we handle ODE semantics, which has the bonus of introducing some new capabilities (#1177) as well as (hopefully) making adding new ODE semantics in the future considerably simpler.
For those reading (or reviewing) this code, my suggested reading order would be:
ode::polynomial_ode::build_system_custom_parametersto see how we interpret models ofth_signed_polynomial_ode_systemas systems of polynomial ODEsode::lotka_volterrato see an example of the main chunk of defining an ODE semantics: giving an implementation forODESemanticsAnalysisode::ode_semanticsto see where all the actual new code isode::mass_actionto see where things are still messy and need some extra finessingFEEDBACK, where I have some specific questions / highlight specific problems with this design.p.s. I know this PR adds quite a lot of code for something called a "refactor", but (a) I added a lot of documentation and some new tests, and (b) subsequent PRs focussing on the front-end parts should now be possible, allowing for much more code removal.
Now that more ODE analyses have LaTeX equations, we should be stricter about the use of
fmt::Displayonly for debugging.This PR introduces some new types and traits (
Latex,ToLatex,ToLatexWithMap) for dealing with LaTeX expressions and rendering object/morphism names nicely in the frontend. It also includes a bunch of tests for ODE semantics in the wasm layer. Finally, as a bonus, it removes a lot of repetitive code fromcatlog-wasm::src::analysesby definingode_semantics_simulationandode_semantics_equations, enabled by #1251, as well as removing some unnecessary types introduced there (such asLotkaVolterraEquationsData, which is justnull).Again, this refactor adds a lot of lines, but that's almost entirely due to how many nice new tests it adds 👼
As a bonus, this PR fixes the following issues:
fmt::Displayfor LaTeX output #1171Although further refactoring is possible (e.g.
lotka_volterra.tsxandlinear_ode.tsxshare a lot of code), I think that the "correct" solution to this would be to start looking at generating that code from the definitions of the analyses rather than manually refactoring it. But this is now a much larger, rather separate project, so I think it's a bit out of scope.With that in mind, I would suggest that this PR finally completes the last item in #1177 , and thus closes the RFC-0001 parent issue #1098.