Detect cases where there's fewer brownians than equations, but noise still 'diagonal' #2914
Conversation
test/sdesystem.jl
Outdated
| @testset "Diagonal noise, less brownians than equations" begin | ||
| @parameters σ ρ β | ||
| @variables x(tt) y(tt) z(tt) | ||
| @brownian a b | ||
| eqs = [D(x) ~ σ * (y - x) + 0.1a * x, # One brownian | ||
| D(y) ~ x * (ρ - z) - y + 0.1b * y, # Another brownian | ||
| D(z) ~ x * y - β * z] # no brownians -- still diagonal | ||
| @mtkbuild de = System(eqs, tt) | ||
|
|
||
| u0map = [ | ||
| x => 1.0, | ||
| y => 0.0, | ||
| z => 0.0 | ||
| ] | ||
|
|
||
| parammap = [ | ||
| σ => 10.0, | ||
| β => 26.0, | ||
| ρ => 2.33 | ||
| ] | ||
|
|
||
| prob = SDEProblem(de, u0map, (0.0, 100.0), parammap) | ||
| @test solve(prob, SOSRI()).retcode == ReturnCode.Success | ||
| end |
There was a problem hiding this comment.
Tested this example and it failed before this PR.
|
The downstream failure in Catalyst is caused by this, not entirely sure why it failed though it 's a bit weird. I'll try a more explicit version of the check for whether the noise is diagonal. |
|
Okay, I think I figured out the problem, I was iterating over the axes of the noise equations in the wrong order, effectively looking at the transpose of the noise equations. This bug was masked in the test suite, but it reared its head in the Catalyst tests because it creates some reaction systems where the noise matrix is of the form [a 0 0 0 e 0 0 0
0 b 0 0 0 f 0 0
0 0 c 0 0 0 g 0
0 0 0 d 0 0 0 h]and since I was iterating over the transpose of that, I was mistakenly detecting it as diagonal noise. Should be fixed now, and I've updated the non-diagonal noise test to watch for this case and ones like it |
|
Hm, so now the failing Catalyst test is happening because Catalyst does this: p_matrix = zeros(length(get_unknowns(sde_sys)), numreactions(rs))
return SDEProblem(sde_sys, u0map, tspan, pmap, args...; check_length,
noise_rate_prototype = p_matrix, kwargs...)in https://github.com/SciML/Catalyst.jl/blob/v14.1.0/src/reactionsystem_conversions.jl#L767. What this ends up doing is hard-coding into the SDEProblem that the noise is going to be a matrix, rather than diagonal, but that interacts badly with the change added in #2886, which will unconditionally make I've removed that change from #2886 and things seems to work now, we'll see how this test-run goes. I don't think that change was necessary if I understand correctly. |
|
If calling SDEProblem on an SDESystem now infers the prototype optimally we could drop that in Catalyst. |
|
By drop I just mean we won’t pass a prototype. |
|
Yeah, dropping that would be be good because some cases in Catalyst would then get a performance boost, but it's also important that we not break code that does assume a specific form of the prototype like that, so I think the change I made in fe1a3ee is still necessary (though perhaps I misunderstood something, and @AayushSabharwal can chime in as he was the one who added it). |
|
I’ll try to update that next week when I’m back from vacation. |
|
@ChrisRackauckas can you merge this? |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Followup to #2882 and #2886