Test fix #1159
Test fix #1159
Conversation
|
Another failure has appeared, apparently affecting using Catalyst, StochasticDiffEq
model = @reaction_network begin
@observables XY ~ X + Y
(kp,kd), 0 <--> X
(k1,k2), X <--> Y
end
@unpack XY, X, Y, kp, kd, k1, k2 = model
# Creates problem.
u0_vals = [X => 4, Y => 5]
tspan = (0.0, 10.0)
p_vals = [kp => 1.0, kd => 0.1, k1 => 0.25, k2 => 0.5]
sprob = SDEProblem(model,u0_vals, tspan, p_vals)
# tests remake.
rp = remake(sprob; u0 = [Y => 7])
rp[[X, Y]] == [4, 7] # false (should be true).
rp[X] # -7.0 (!, should be 4.0, X is never changed)
rp[Y] # 7.0 (correct)
oprob = ODEProblem(model,u0_vals, tspan, p_vals)
rp = remake(oprob; u0 = [Y => 7])
rp[X] # 4.0, correct.I will raise with MTK |
|
A different case, where we cannot even call using Catalyst, StochasticDiffEq
model = @reaction_network begin
(p1,2.0), 0 <--> X
(p2,2.0), 0 <--> Y
end
@unpack X, Y, p1, p2 = model
# Creates problem.
u0_vals = [X => 4, Y => 5]
tspan = (0.0, 10.0)
p_vals = [p1 => 1.0, p2 => 0.1]
sprob = SDEProblem(model,u0_vals, tspan, p_vals)
# tests remake.
rp = remake(sprob; u0 = [Y => 7] # ERROR: Initialization incomplete. Not all of the state variables of the DAE system can be determined by the initialization. |
|
MTK issue: SciML/ModelingToolkit.jl#3295 |
|
Another one! Turns out that parameter defaults are broken for using Catalyst
model = @reaction_network begin
@parameters Y0
@species Y(t) = Y0
(k1,k2), X <--> Y
end
@unpack X, Y, Y0, k1, k2 = model
u0 = [X => 4]
tspan = (0.0, 10.0)
p = [k1 => 0.25, k2 => 10, Y0 => 2]
oprob = ODEProblem(model, u0, tspan, p) # Works.
sprob = SDEProblem(model, u0, tspan, p) # Works.
dprob = DiscreteProblem(model, u0, tspan, p) # Errors. |
|
What error does that spit out? |
|
|
@TorkelE probably good to add these examples and their errors messages to the tracking MTK issue? |
|
New error, but this one seems codecov related (I am trying what happens if I just rerun it) |
|
What's the new HC doc build failure? |
|
Not just HC (didn't see that one, but some in nonlinear solve and symbolic stochiometry tests), but we suddenly got a lots of errors, including stuff that passed an hour ago. I re-ran everything again to see what happens. |
|
Yeah, the symbolic stoichiometry tests are broken now. Which is funny as these are before the MTK tests that were failing (and fixes), so we shouldn't even have gotten there if those weren't working. I tried to reproduce locally, but worked fine. Pkg marked everything as up to date, but maybe there has been a very recent release which CI pulls but my local Pkg is unaware of. |
|
You're not looking at the generated code right? The only release of today is a fix to change ^ to NaNMath.pow, but that shouldn't break anything except tests that expect ^ specifically |
|
Not sure really. I have a bunch of other stuff to sort out, and there is already another bunch of tests failing. Will have a look at this again tomorrow, if I cannot reproduce locally then I will have to look into it deeper. |
|
Cannot see any HC problems, but what is causing issues is stuff like and I think we had some problem like this long ago, but should have been sorted by now. |
|
I'll make another PR later today to cap MTK at the previous release to see if that fixes stuff (and hopefully allow us to work on other PRs). |
|
Sounds like jump initial conditions or integer parameters are now getting promoted to floating point? |
|
Sounds like a good guess, hopefully we'll know for sure soon. |
|
Are you forcing split=false? |
|
We don't do anything special like that in Catalyst. |
|
For the SDE using Catalyst, StochasticDiffEq
model = @reaction_network begin
@observables XY ~ X + Y
(kp,kd), 0 <--> X
(k1,k2), X <--> Y
end
@unpack XY, X, Y, kp, kd, k1, k2 = model
# Creates problem.
u0_vals = [X => 4, Y => 5]
tspan = (0.0, 10.0)
p_vals = [kp => 1.0, kd => 0.1, k1 => 0.25, k2 => 0.5]
sprob = SDEProblem(model,u0_vals, tspan, p_vals)
# tests remake.
rp = remake(sprob; u0 = [Y => 7])
rp[[X, Y]] == [4, 7] # false (should be true).
rp[X] # -7.0 (!, should be 4.0, X is never changed)
rp[Y] # 7.0 (correct)I don't yet know how, but this is the equations in the initialization system before Y(t) ~ 7
XY(t) ~ Y(t) + X(t)And after: Y(t) ~ 7.0
XY(t) ~ -0.0
XY(t) ~ Y(t) + X(t)Somehow it's adding EDIT: the bug is in |
|
Can we turn the alias elimination issue into an MTK issue to track? |
|
|
Ok, there's the issue ou mentioned again Sam. Still cannot reproduce locally even when I think I am on the same environment, so might take a bit to figure out. |
|
I tried to mark the latest version (which we are testing against). Figured |
|
No, the goal is to not have master be capped. Otherwise we won't see when things break. The point is to cap releases so they can't end up getting versions of MTK/SciMLBase that don't work with them. |
|
Sounds good |
|
(I only capped master yesterday so we could get all these PRs that are building up merged until the fixes you've raised issues about here are merged.) |
|
This was the problem you were talking about: #1170 (comment), but seems to dealt with it (for now)already) |
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
|
I'm not seeing that it is fixed? I'm getting errors about it as of my most recent PR today. |
|
No, it is not fixed. My commented was that the problem is still there, but I could not reproduce locally so might not be able to make an immediate fix. Then I referenced #1170 meaning that you were already taking care of it (as it commenting it out for now, although I referenced the issue, not the PR). |
|
Got it. |
|
Could it be you are using different package versions than the doc builds? |
|
Something is definitely off. I have some other stuff I really need to sort out asap, but will sit down to have a closer look at it. |
|
Ok, this one is ready now |
Turns out that MTK now gives over/under determined system warnings for certain cases when SDEProblems are generated. What is slightly odd is that this is not the case for the corresponding ODEs (does this make sense @AayushSabharwal ?).
We had some tests which depended on checking warnings for when conserved quantities are eliminated. These checks no longer worked when MTK threw additional warnings. I have removed the MTK warnings. This makes tests pass, but we might want to check why these warnings are actually generated.