WIP: fixes for MTK changes #1186
Conversation
|
This is probably never going to pass tests with the change in semantics for |
|
We can also no longer rely on |
|
Let's see how this turns out. If something like what you suggest is really changing it would be one of the largest changes to Catalyst ever, probably after moving to maps as inputs to problems. Would probably take ages to figure out all the implications. |
|
@TorkelE let's just work in this PR to try to get tests working again. I've handled the conservation law constants getting scalarized issue, and locally gotten tests in reactionsystem.jl working again. |
|
TODO:
|
|
I had some updates locally to sort out the new parameters, but there are quite a few recent MTK issues that are hard-blocking us so I stopped a bit: |
|
OK, I'll get as much fixed here as I can modulo those issues and then plan to merge. FYI, take a look at the changes to |
|
@TorkelE everything before conservation laws should now work. I tried various combinations of guesses and missing but couldn’t get them to work. Are you ok if I merge this and we can resume if/when someone figures out how to handle that stuff? |
src/reactionsystem_conversions.jl
Outdated
| elseif has_alg_equations(rs) | ||
| error("The input ReactionSystem has algebraic equations. This requires setting `structural_simplify=true` within `ODEProblem` call.") | ||
| # elseif has_alg_equations(rs) | ||
| # error("The input ReactionSystem has algebraic equations. This requires setting `structural_simplify=true` within `ODEProblem` call.") |
There was a problem hiding this comment.
ANy reason to remove this? structural_simplify is still a mess, but this still seems like a useful error to throw?
There was a problem hiding this comment.
What's the issue with structural simplify? I don't know of one related to Catalyst
There was a problem hiding this comment.
Wasn't there the cases where it may or may not simplify things into DAEs. However, depending on whichever variable was reduced to an algebraic equation, the user now have to determine whether its initial condition should go as an initial condition or a guess. And where the user have to explicitly check the simplified system to figure out which ended up being the case?
There was a problem hiding this comment.
No and no. First of all, that is no issue of structural_simplify, this is just a property of DAEs and index 1 transformations 😅 . So if you're waiting for that to ever change, sorry but it's just the math of DAEs. I'd be happy to hop on a call and explain it since you and @isaacsas keep asking the same question, it's probably just easiest to pull out a whiteboard at this point. But it shouldn't cause any difficulties to the user.
Here's a simple way to understand it. If your DAE is index 1 (which it will be after transformation), then every equation is either a differential equation or an algebraic equation, and every equation is associated through matching to be solving for one variable (this is equivalent to the index 1 condition and not having a singularity in the Jacobian). So with this, any variable can be called either a differential or algebraic variable. You need as many initial conditions as you have algebraic variables, and any variable that doesn't have an initial condition needs a guess. It doesn't matter which one is chosen by the system as differential or algebraic, but it does matter that you have the right number of conditions. If you have more conditions, it throws you a warning because it's generally not a good idea to have an overdetermined system, but it doesn't mean it's impossible to solve. If it's over or underdetermined it will still try to initialize, with many caveats of course.
The simplest way to understand this then is that, every variable needs either an initial condition or a guess. You should probably be making every differential variable have an initial condition, and for every algebraic condition you should be introducing a new variable and give that one a guess.
There is potentially a way to not need guesses if we move beyond Newton methods to more homotopy type solvers, since it's really just a property of Newton that you need a u0, but this'll be years of research and not something to rely on.
There was a problem hiding this comment.
I just commented this out while debugging. I didn't intend to keep it commented out when merging...
| @@ -1468,3 +1469,5 @@ let | |||
| @observables X2 ~ X1 | |||
| end | |||
| end | |||
|
|
|||
| nothing | |||
There was a problem hiding this comment.
The script was outputting a very long expression to the REPL from one of the tests for some reason.
There was a problem hiding this comment.
maybe move it to the end of the last let ... end block, just to make it clear that it belongs there (at least if that is the block which returns the last output, but I presume that would bet he case?).
|
If we want to merge this I am happy with it. Still quite a way to go to update to the latest everything, but not sure if it is possible yet. |
|
No, we can keep this open. But it has a lot of fixes for the MTK changes, so we should just push to this PR as we work on tests going forward... |
|
sounds good, thought from your latest comment that we maybe merge this and then proceed, but happy to continue using this branch. |
Updates conservation laws to work with new
remakeversion.