Remake for conservation laws update #1203
Conversation
|
I have an doc update, as well, but it requires #1205 |
| @unpack X1, X2, X3 = rn | ||
| u0 = [X1 => 1.0, X2 => 1.0, X3 => 1.0] | ||
| ps = [:k1 => 0.1, :k2 => 0.2, :k3 => 0.3, :k4 => 0.4] | ||
| prob_old = ODEProblem(rn, u0, 1.0, ps; remove_conserved = true) |
There was a problem hiding this comment.
What about other relevant problem types (i.e, NonlinearProblem or SteadyStaeProblem)?
There was a problem hiding this comment.
Once we have it working for NonlinearSystems I will add that one in a well here. I can do the other 2, however, this test block takes almost 3 seconds, so it will be like 5-6 second per test run (is this worth it?)
There was a problem hiding this comment.
I think it's worth it. Correctness should be our top aim, even if it means slower tests.
There was a problem hiding this comment.
Sounds good. And at some point we can paralelise tests as well, at which point the time will be even less of an worry.
Co-authored-by: Sam Isaacson <[email protected]>
…remake_test_n_warn_update
|
I have added |
|
Sounds good. LMK when this is done then (i.e. no longer WIP). |
TorkelE
changed the title
| @@ -39,7 +39,7 @@ Using the `unknowns` function we can confirm that the ODE only has a single unkn | |||
| ```@example conservation_laws | |||
| unknowns(osys) | |||
| ``` | |||
| Next, using `parameters` we note that an additional parameter, `Γ[1]` has been added to the system: | |||
There was a problem hiding this comment.
Due to MTK updates, the conservaton law parameters are not displayed as Γ[1:1] anymore, but only Γ.
(I prefered the old way and have raised an issue about it)
| @@ -1,5 +1,5 @@ | |||
| # [Working with Conservation Laws](@id conservation_laws) | |||
| Catalyst can detect, and eliminate for differential-equation based models, *conserved quantities*, i.e. linear combinations of species which are conserved via the chemistry. This functionality is both automatically utilised by Catalyst (e.g. to [remove singular Jacobians during steady state computations](@ref homotopy_continuation_conservation_laws)), but is also available for users to utilise directly (e.g. to potentially [improve simulation performance](@ref ode_simulation_performance_conservation_laws)). | |||
There was a problem hiding this comment.
think this is more correct
|
|
||
| !!! note | ||
| The `remove_conserved = true` option is available when creating `SDEProblem`s, `NonlinearProblem`s, and `SteadyStateProblem`s (and their corresponding systems). However, it cannot be used when creating `JumpProblem`s. | ||
|
|
||
| !!! warn | ||
| Users of the [ModelingToolkit.jl](https://github.com/SciML/ModelingToolkit.jl) package might be familiar with the `structural_simplify` function. While it can be applied to Catalyst models as well, generally, this should be avoided (as `remove_conserved` performs a similar role, but is better adapted to these models). Furthermore, applying `structural_simplify` will interfere with conservation law removal, preventing users from accessing eliminated quantities. |
There was a problem hiding this comment.
Is it actually worse than this, i.e. does structural_simplify drop Observed for ODEs, and hence lead to incorrect models? If so, the warning should be more strict about not mixing it with the use of conservation law elimination.
There was a problem hiding this comment.
So, initially I wrote this. But then I tried to reproduce the removal of observables, and it didn't seem to be a case anymore. At that point things thing had gone so much around in my head I just closed it down. I should try and go to the bottom with it though. Worst case there should not be a silent error (people will just not be able to retrieve the observable values). Happy to say this in the warning, and then we can remove it if things turn out fine.
There was a problem hiding this comment.
We should probably add tests about this (i.e. check that structural_simplify converted models don't drop observed). If that works now it would be good to have tests suddenly start failing should MTK change the behavior so we know it has changed.
Co-authored-by: Sam Isaacson <[email protected]>
|
100% agree with the proposed API functions. Want me to add them to this PR, or as a follow-up? Might also be nice to make DSL/ |
|
Adding the API functions here and updating the docs accordingly sounds good. Maybe the kwarg should just be |
|
I also wonder if we should stick with immutability more, and make conservation law stuff a transformation that generates a new system (that can have gamma as a parameter). Still will be a bit clunky though since the user won't always have that system returned to them if it is done via |
|
An operation on e.g. |
|
Let's leave it for a future PR. I forgot that we do conservation laws on the flattened system, so this info isn't getting stored in the original user reaction system. We'd probably need to make conversation law removal be at completion time for the user to have a reaction system with the symbol in it. |
|
Feel free to merge if you are happy with this. |
|
will merge when tests pass |
Updates docs, history file, and removes warnings, to note users that this now should work.
Still needs further expanded tests to guarantee that this is actually the case.