Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catalyst V15 todo #1097

Open
isaacsas opened this issue Oct 28, 2024 · 18 comments
Open

Catalyst V15 todo #1097

isaacsas opened this issue Oct 28, 2024 · 18 comments
Assignees

Comments

@isaacsas
Copy link
Member

isaacsas commented Oct 28, 2024

  • Uncap SciMLBase, DiffEqBase (See Uncap those bases #1127)
  • Some of the mtk_structure_indexing.jl when using Symbols (broken with SciMLBase > 2.57.1) (Seems fixed now)
  • Stability computation (broken with SciMLBase > 2.57.1, currently disabled)
  • Serialisation (disabled)
  • StructuralIdentifiability (disabled, add when works on 10.5 and up)
  • spatial_modelling/lattice_reaction_systems.jl (disabled)
  • Bifkit disabled due to crashes.
  • add rebuild
  • update HISTORY.md and docs with changes (rebuild for example).
  • fix Latex display?
@isaacsas isaacsas added the bug label Oct 28, 2024
@isaacsas isaacsas changed the title Serialisation tests failing so disabled Summary of currently broken or disabled tests Oct 28, 2024
@isaacsas
Copy link
Member Author

The first two seem to be due to SciML/SciMLBase.jl#832

@isaacsas
Copy link
Member Author

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

@AayushSabharwal
Copy link
Member

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
Copy link
Member

SciML/SciMLBase.jl#837

@isaacsas
Copy link
Member Author

isaacsas commented Nov 4, 2024

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:

# @time @safetestset "ReactionSystem Serialisation" begin include("miscellaneous_tests/reactionsystem_serialisation.jl") end

#@time @safetestset "Lattice Reaction Systems" begin include("spatial_modelling/lattice_reaction_systems.jl") end

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:

@time @safetestset "MTK Structure Indexing" begin include("upstream/mtk_structure_indexing.jl") end
@time @safetestset "MTK Problem Inputs" begin include("upstream/mtk_problem_inputs.jl") end

When uncapping it this was also failing for me:

@time @safetestset "Steady State Stability Computations" begin include("extensions/stability_computation.jl") end

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
Copy link
Member

"ReactionSystem Serialization" test failures are due to the changes in SciML/ModelingToolkit.jl#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:

  • Only flatten if the system has a parent field. This would still duplicate parameters, and rely on the unique call.
  • Add the parent field to ReactionSystem

@AayushSabharwal
Copy link
Member

In "Lattice Reaction Systems", the only failure is

@test_throws ArgumentError LatticeReactionSystem(rs2, [TransportReaction(D, X)], CartesianGrid((2,2)))
. I'm not sure what error that's testing and why it should error.

@isaacsas
Copy link
Member Author

isaacsas commented Nov 5, 2024

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
Copy link
Member

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
Copy link
Member

MTK structure indexing tests are fixed on SciMLBase#master

@isaacsas
Copy link
Member Author

isaacsas commented Nov 5, 2024

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
Copy link
Member

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 (

# @test base_sol == solve(oprob, Tsit5(); saveat = 1.0)
) but this won't be the case if the parameters are different.

@AayushSabharwal
Copy link
Member

Will that get a fix then in MTK?

Yes, I can PR

@isaacsas
Copy link
Member Author

isaacsas commented Nov 5, 2024

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 (

# @test base_sol == solve(oprob, Tsit5(); saveat = 1.0)

) but this won't be the case if the parameters are different.

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

@AayushSabharwal
Copy link
Member

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
Copy link
Member

TorkelE commented Nov 6, 2024

@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
Copy link
Contributor

rveltz commented Nov 15, 2024

Hi,

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

@isaacsas
Copy link
Member Author

@rveltz thanks! We've been trying to get it working / merged in #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).

@isaacsas isaacsas changed the title Summary of currently broken or disabled tests Catalyst V15 todo Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants