Closed ErikQQY closed 1 year ago
This PR seems to fail a lot of tests.
Now these failings seem caused by SDEProblemLibrary.jl
It seems the testing for StochasticDiffEq.jl is still using the old version of SDEProblemLibrary.jl, is there some way we can bump it?
Up the lower bound.
The lower bound of SDEProblemLibrary.jl in tests? I don't think there is one.
Looks like it got the updated SDEProblemLibrary
Yeah, it is using the latest SDEProblemLibrary, but the error is still about Catalyst and MTK stuff, is there something I am missing?
No, those test errors are real: https://github.com/SciML/StochasticDiffEq.jl/actions/runs/6291363683/job/17079673649?pr=546#step:6:548
Was the tests all passing locally? That seems unlikely.
The errors just remind me that the change of SDEProblem
constructor also affects SplitSDEProblem
and etc., may need a little longer to get all this fixed. For example, see
If we are gonna change the SDEProblem
constructor, we need to take care of these too
Just the constructor didn’t cover all the cases, need further modifications.
Did you forget to push an overload to remake
like https://github.com/SciML/SciMLBase.jl/blob/master/src/remake.jl#L52 ?
Yeah, I need to make a dispatch of remake
for SDEProblem
Tests failings are real, still need some modifications on SplitSDEProblem
and DynamicalSDEProblem
Finally all the issues are addressed🎉🎉🎉
on master https://github.com/SciML/StochasticDiffEq.jl/pull/549 but on this PR:
CPU Weak adaptive step size Brusselator \| 1 1 11.4s -- | 11.764316 seconds (25.83 M allocations: 2.619 GiB, 6.65% gc time, 553.30% compilation time) | i = 1 | 1959.171435 seconds (160.16 M allocations: 12.123 GiB, 0.64% gc time, 14.31% compilation time) | err1 = 0.0027647512f0 | 1955.643607 seconds (156.90 M allocations: 11.920 GiB, 0.58% gc time) | err2 = 0.0019452827f0 | 1953.844537 seconds (157.60 M allocations: 11.980 GiB, 0.56% gc time) | err3 = 0.0012469663f0 | | i = 2 | 1955.429909 seconds (161.64 M allocations: 12.211 GiB, 0.52% gc time, 9.78% compilation time) | err1 = 6.1122984f-5 | 1953.194058 seconds (159.90 M allocations: 12.107 GiB, 0.50% gc time) | err2 = 6.112158f-5 | 1925.533936 seconds (159.90 M allocations: 12.107 GiB, 0.50% gc time) | err3 = 3.861361f-5 | | i = 1 | 1951.627145 seconds (147.74 M allocations: 11.316 GiB, 0.45% gc time, 14.63% compilation time) | err1 = 0.0021299827f0 | # Received cancellation signal, interrupting | | [514] signal (15): Terminated | in expression starting at none:11Interesting, that looks like it was just a random hiccup.
It seems changing the constructor makes it much slower, but I tested on SciMLBase.jl 1.70.0 and SciMLBase 2.06 on a same problem and I didn't see significant performance loss:
SciMLBase 1.70.0:
SciMLBase 2.0.6:
I have a suggestion:
It would be great if we had PkgBenchmark
setup, we could have BenchmarkCI
to generate a report for each PR. improving performance with an independent report is more convenient. And we would be sure that PRs aren't detrimental to performance.
I am interested to create it, but I don't know where to start.
It looks like DiffEqNoiseProcess's regression is on master, so this can be merged. https://github.com/SciML/StochasticDiffEq.jl/actions/runs/6353130403/job/17257204276?pr=549
It would be great if we had PkgBenchmark setup, we could have BenchmarkCI to generate a report for each PR. improving performance with an independent report is more convenient. And we would be sure that PRs aren't detrimental to performance. I am interested to create it, but I don't know where to start.
Sure that would be cool, but here it was just a quirk in the CI machine.
Now the SDE constructor is simply
SDEProblem(SDEFunction(f, g), u0, tspan)
orSDEProblem(f, g, u0, tspan)
https://github.com/SciML/SciMLBase.jl/pull/489