search

SciML / Catalyst.jl

Chemical reaction network and systems biology interface for scientific machine learning (SciML). High performance, GPU-parallelized, and O(1) solvers in open source software.
https://docs.sciml.ai/Catalyst/stable/
Other
464 stars 78 forks source link

Catalyst V15 todo #1097

Open isaacsas opened 3 weeks ago

isaacsas commented 3 weeks ago
isaacsas commented 3 weeks ago

The first two seem to be due to https://github.com/SciML/SciMLBase.jl/pull/832

isaacsas commented 3 weeks ago

I’ve capped SciMLBase till the remake issues are fixed.

AayushSabharwal commented 3 weeks ago

https://github.com/SciML/Catalyst.jl/actions/runs/11560966175/job/32178950735?pr=1098#step:6:1033

The breakage here is because p is a map from Symbol to value. u0 contains the mapping Z => Z0, and p contains :Z0 => 10.0, but it can't substitute the Symbol into the BasicSymbolic. This used to work because remake would reason that since the values in p are all numbers (no symbolics) it can create the parameter buffer, and then use getu to get the values in u0. That approach was abandoned because if p contains a value for a parameter dependency, it won't necessarily be reflected in the parameter object and any u0 depending on it would get an incorrect value. This is analogous to the mass conservation case where Gamma[1] => X + Y, but Y is an observed variable and thus not present in the instantiated u0 vector, leading to incorrect values for Gamma.

I'll try and think of a way to resolve this.

AayushSabharwal commented 3 weeks ago

https://github.com/SciML/SciMLBase.jl/pull/837

isaacsas commented 2 weeks ago

Chris asked me to update this post as of today (11/4). Here is the current state of things as I understand it:

Serialization and spatial are disabled in tests, but were both failing when I last tested them:

https://github.com/SciML/Catalyst.jl/blob/bef456642f7a720c56849f6305715d433d18d3ca/test/runtests.jl#L60

https://github.com/SciML/Catalyst.jl/blob/bef456642f7a720c56849f6305715d433d18d3ca/test/runtests.jl#L77

I capped SciMLBase at 2.57.1 in the Project.toml so we could still get stuff merged. If it is uncapped then one or both of these were failing for me:

https://github.com/SciML/Catalyst.jl/blob/bef456642f7a720c56849f6305715d433d18d3ca/test/runtests.jl#L54-L55

When uncapping it this was also failing for me:

https://github.com/SciML/Catalyst.jl/blob/bef456642f7a720c56849f6305715d433d18d3ca/test/runtests.jl#L95

The MTK indexing test failures were symbol-related issues I think (i.e. using symbols instead of symbolics), maybe remake too. The stability computation stuff I didn’t into dig much but I think it was breaking on remake and/or initialization related stuff for the conservation laws (but it previously worked on 2.57.1 and earlier). Neither of these is actually disabled currently as they work with SciMLBase <= 2.57.1.

AayushSabharwal commented 2 weeks ago

"ReactionSystem Serialization" test failures are due to the changes in https://github.com/SciML/ModelingToolkit.jl/pull/3149. Specifically, sys.subsys.var syntax requires the subsystems to be present in MTK.get_systems(sys) or MTK.get_systems(MTK.get_parent(sys)). flatten(sys::ReactionSystem) loses the systems information, and since ReactionSystem doesn't have a parent field the latter approach doesn't work either.

complete now calls flatten because it's the correct approach to enable the parameter reordering it does subsequently. Without it, parameters of subsystems end up duplicated in the root system. The only reason parameters(sys) still works is because it calls unique on the returned vector.

There are two ways to fix this:

AayushSabharwal commented 2 weeks ago

In "Lattice Reaction Systems", the only failure is https://github.com/SciML/Catalyst.jl/blob/bef456642f7a720c56849f6305715d433d18d3ca/test/spatial_modelling/lattice_reaction_systems.jl#L260. I'm not sure what error that's testing and why it should error.

isaacsas commented 2 weeks ago

I hadn't seen the change that complete now flattens. That is nice in that we can now get rid of lots of places where we flatten (like in system conversion or conservation law detection).

Specifically, sys.subsys.var syntax requires the subsystems to be present in MTK.get_systems(sys) or MTK.get_systems(MTK.get_parent(sys)). flatten(sys::ReactionSystem) loses the systems information, and since ReactionSystem doesn't have a parent field the latter approach doesn't work either.

We can update ReactionSystems to have parent fields. Beyond adding the field what do we need to do with it here (for example, we have our own dispatches of flatten, compose and extend, do they need to do anything with it?).

Also though, isn't this change in indexing behavior after calling complete breaking (i.e. it was added post 9.0.0 right?).

AayushSabharwal commented 2 weeks ago

Beyond adding the field what do we need to do with it here (for example, we have our own dispatches of flatten, compose and extend, do they need to do anything with it?).

They shouldn't have anything to do with it. complete denotes that the system won't be modified, so compose and extend are immaterial at that point.

Also though, isn't this change in indexing behavior after calling complete breaking

Yeah it was accidental. Instead of calling flatten then checking has_parent, it should only have flattened if has_parent.

AayushSabharwal commented 2 weeks ago

MTK structure indexing tests are fixed on SciMLBase#master

isaacsas commented 2 weeks ago

Yeah it was accidental. Instead of calling flatten then checking has_parent, it should only have flattened if has_parent.

Will that get a fix then in MTK? Otherwise that bug will still impact released Catalyst versions.

MTK structure indexing tests are fixed on SciMLBase#master

OK, great. I'm swamped today but if I have time tomorrow I will test against MTK and SciMLBase masters to see where things are at then.

AayushSabharwal commented 2 weeks ago

With MTK problem inputs tests, why does the value of p keep changing in https://github.com/SciML/Catalyst.jl/blob/master/test/upstream/mtk_problem_inputs.jl#L258? Later, you test that the solution of the problem from remake is identical to the original solution (https://github.com/SciML/Catalyst.jl/blob/bef456642f7a720c56849f6305715d433d18d3ca/test/upstream/mtk_problem_inputs.jl#L303) but this won't be the case if the parameters are different.

AayushSabharwal commented 2 weeks ago

Will that get a fix then in MTK?

Yes, I can PR

isaacsas commented 2 weeks ago

With MTK problem inputs tests, why does the value of p keep changing in https://github.com/SciML/Catalyst.jl/blob/master/test/upstream/mtk_problem_inputs.jl#L258? Later, you test that the solution of the problem from remake is identical to the original solution (

https://github.com/SciML/Catalyst.jl/blob/bef456642f7a720c56849f6305715d433d18d3ca/test/upstream/mtk_problem_inputs.jl#L303 ) but this won't be the case if the parameters are different.

@TorkelE can you explain what you were doing there to @AayushSabharwal.

AayushSabharwal commented 2 weeks ago

I hadn't seen the change that complete now flattens. That is nice in that we can now get rid of lots of places where we flatten (like in system conversion or conservation law detection).

In case you're planning to do this soon, I would hold off a bit. There are some breakages we don't have tests for that put into question the validity of the changes to complete.

TorkelE commented 2 weeks ago

@AayushSabharwal I think you are right that there is something bad with that test. I will go through it properly and see if I can fix it, but Iäd ignore it for now as it seems incorrect.

rveltz commented 1 week ago

Hi,

Just to mention that I tagged BifurcationKit at 0.4.4 for fixing pre-compilation

isaacsas commented 1 week ago

@rveltz thanks! We've been trying to get it working / merged in https://github.com/SciML/Catalyst.jl/pull/1101 but it seems to have become a game of whack a mole -- every time something gets fixed a new problem crops up (I don't think the errors we are getting now are BifurcationKit specific, but due to other changes in MTK).