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

Uncap those bases #1127

Merged
merged 7 commits into from
Nov 22, 2024
Merged

Uncap those bases #1127

merged 7 commits into from
Nov 22, 2024

Conversation

isaacsas
Copy link
Member

Let's see where things now stand.

@isaacsas
Copy link
Member Author

@TorkelE this looks like the first failure in the tests on the latest SciMLBase / DiffEqBase:

osol_ordered = let
@variables M(t) H(t)=h_max
@species S(t) I(t) R(t)
eqs_ordered = [
Reaction(i, [S, I], [I], [1, 1], [2]),
Reaction(r, [I], [R]),
D(M) ~ -I*M/(m1 + m2),
H ~ h_max - I
]
@named coupled_sir_ordered = ReactionSystem(eqs_ordered, t)
coupled_sir_ordered = complete(coupled_sir_ordered)
# Checks that ODE an simulation of the system achieves the correct steady state.
oprob_ordered = ODEProblem(coupled_sir_ordered, u0, tspan, ps; structural_simplify = true)
solve(oprob_ordered, Vern7(); abstol = 1e-8, reltol = 1e-8, saveat = 1.0)
end
seems like something related to initialization and DAEs.

@ChrisRackauckas
Copy link
Member

What is that doing? It says it's testing something in the comments but there is no test?

@ChrisRackauckas
Copy link
Member

@AayushSabharwal that looks like a bug, state values shouldn't be called on the odefunction but on the prob

@isaacsas
Copy link
Member Author

Hopefully @TorkelE can clarify but I guess it is ultimately testing two different model formulations give the same answers.

@TorkelE
Copy link
Member

TorkelE commented Nov 20, 2024

The test are a little bit below. basically, the same model is declared twice, but me basically scrambling up the declaration as much as possible. Since the models are not actually different, their simulations should be identical, which does not seem to be the case anymore. Will have a look. Should be relatively straightforward to remove stuff until they are identical, and figure out which step do not work anymore.

@AayushSabharwal
Copy link
Member

AayushSabharwal commented Nov 21, 2024

that looks like a bug, state values shouldn't be called on the odefunction but on the prob

Yeah. SciML/OrdinaryDiffEq.jl#2541 fixes it, but I'm still figuring out how none of the tests caught that. Does OrdinaryDiffEq not test initialize_dae! anywhere?

@AayushSabharwal
Copy link
Member

Yeah so OrdinaryDiffEq doesn't test OverrideInit at all, it should have lit up red for multiple reasons

@ChrisRackauckas
Copy link
Member

Only MTK hits overrideinit since it requires building the init system to test it. We probably want to section those out so that SciMLBase can have that in its downstream tests.

@AayushSabharwal
Copy link
Member

AayushSabharwal commented Nov 21, 2024

SciMLBase has the entirety of MTK's tests in its downstream. The reason this wasn't detected is because when the initialization was moved to SciMLBase, OrdinaryDiffEq didn't use it so MTK didn't error. When OrdinaryDiffEq was made to use the shifted initialization, it didn't have MTK in its downstream so none of the OverrideInit stuff was hit.

@ChrisRackauckas
Copy link
Member

I see, that's just a twisted circle

@isaacsas
Copy link
Member Author

isaacsas commented Nov 22, 2024

@TorkelE this:

https://github.com/SciML/Catalyst.jl/actions/runs/11976784765/job/33393242914#step:6:1726

is the stability failure I mention in the TOFIX issue. I think that may be the last item on the list besides rebuild (but perhaps it needs rebuild?).

Can you take a look at it? I think I'll merge this unless you object and we can get that fixed in a followup?

@isaacsas isaacsas mentioned this pull request Nov 22, 2024
@TorkelE
Copy link
Member

TorkelE commented Nov 22, 2024

Yes, let's merge this and I will get to the stability thing over the weekend (away tomorrow, but will fix it on Sunday). The spatial one is under way, and the serialization one is ready once I have figured out the new way hierarchical systems are stored. Once I have that figured out I might add some normal tests to our file to ensure that is fully covered in the future as well.

@isaacsas isaacsas merged commit 2cd8c75 into master Nov 22, 2024
10 of 13 checks passed
@isaacsas isaacsas deleted the isaacsas-uncap-bases branch November 22, 2024 22:18
@ChrisRackauckas
Copy link
Member

is the stability failure I mention in the TOFIX issue. I think that may be the last item on the list besides rebuild (but perhaps it needs rebuild?).

SteadyStateProblem / NonlinearProblem just needs to be setup to handle the initialization system, which should be done shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants