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

Refactor SDEProblem constructor #546

Merged
merged 12 commits into from
Sep 29, 2023
Merged

Conversation

ErikQQY
Copy link
Member

@ErikQQY ErikQQY commented Sep 22, 2023

Now the SDE constructor is simply SDEProblem(SDEFunction(f, g), u0, tspan) or SDEProblem(f, g, u0, tspan)

SciML/SciMLBase.jl#489

@ChrisRackauckas
Copy link
Member

This PR seems to fail a lot of tests.

Signed-off-by: ErikQQY <[email protected]>
@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 23, 2023

Now these failings seem caused by SDEProblemLibrary.jl

Signed-off-by: ErikQQY <[email protected]>
@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 23, 2023

It seems the testing for StochasticDiffEq.jl is still using the old version of SDEProblemLibrary.jl, is there some way we can bump it?

@ChrisRackauckas
Copy link
Member

Up the lower bound.

@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 23, 2023

The lower bound of SDEProblemLibrary.jl in tests? I don't think there is one.

@ErikQQY ErikQQY closed this Sep 24, 2023
@ErikQQY ErikQQY reopened this Sep 24, 2023
@ErikQQY ErikQQY closed this Sep 24, 2023
@ErikQQY ErikQQY reopened this Sep 24, 2023
@ChrisRackauckas
Copy link
Member

Looks like it got the updated SDEProblemLibrary

@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 25, 2023

Yeah, it is using the latest SDEProblemLibrary, but the error is still about Catalyst and MTK stuff, is there something I am missing?

@ChrisRackauckas
Copy link
Member

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.

Project.toml Outdated Show resolved Hide resolved
@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 26, 2023

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.

Project.toml Outdated Show resolved Hide resolved
@ChrisRackauckas
Copy link
Member

Did you forget to push an overload to remake like https://github.com/SciML/SciMLBase.jl/blob/master/src/remake.jl#L52 ?

@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 27, 2023

Yeah, I need to make a dispatch of remake for SDEProblem

Project.toml Outdated Show resolved Hide resolved
@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 28, 2023

Tests failings are real, still need some modifications on SplitSDEProblem and DynamicalSDEProblem

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 29, 2023

Finally all the issues are addressed🎉🎉🎉

@ChrisRackauckas
Copy link
Member

[ Info: Brusselator

  | 4.664995 seconds (14.82 M allocations: 1.955 GiB, 9.68% gc time, 134.71% compilation time)
  | Test Summary: | Pass Total Time
  | CPU Weak adaptive step size Brusselator | 1 1 11.8s
  | 11.945955 seconds (25.94 M allocations: 2.641 GiB, 7.33% gc time, 59.82% compilation time)
  | i = 1
  | 13.370246 seconds (126.87 M allocations: 8.381 GiB, 16.19% gc time, 257.21% compilation time)
  | err1 = 0.0027647512f0
  | 9.831222 seconds (123.42 M allocations: 8.167 GiB, 16.98% gc time)
  | err2 = 0.0019452827f0
  | 9.792364 seconds (124.14 M allocations: 8.227 GiB, 18.13% gc time)
  | err3 = 0.0012469663f0
  |  
  | i = 2
  | 12.227704 seconds (128.26 M allocations: 8.463 GiB, 13.56% gc time, 273.88% compilation time)
  | err1 = 6.1122984f-5
  | 9.028020 seconds (126.44 M allocations: 8.355 GiB, 19.57% gc time)
  | err2 = 6.112158f-5
  | 9.339772 seconds (126.43 M allocations: 8.355 GiB, 17.84% gc time)
  | err3 = 3.861361f-5
  |  
  | i = 1
  | 10.933722 seconds (114.33 M allocations: 7.568 GiB, 13.74% gc time, 344.41% compilation time)
  | err1 = 0.0021299827f0
  | 9.305304 seconds (112.86 M allocations: 7.487 GiB, 18.45% gc time)
  | err2 = 0.0013530719f0
  |  
  | i = 2
  | 10.002936 seconds (113.88 M allocations: 7.547 GiB, 15.18% gc time, 193.06% compilation time)
  | err1 = 6.401297f-5
  | 8.078128 seconds (112.43 M allocations: 7.460 GiB, 18.62% gc time)
  | err2 = 4.1367533f-5
  |  
  | Test Summary: | Pass Total Time
  | CPU Weak adaptive | 6 6 1m43.6s
  | 103.623537 seconds (1.21 G allocations: 80.158 GiB, 16.35% gc time, 122.10% compilation time)
  | 115.600149 seconds (1.24 G allocations: 82.800 GiB, 15.41% gc time, 115.65% compilation time)

on master #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:11

@ChrisRackauckas
Copy link
Member

Interesting, that looks like it was just a random hiccup.

@ErikQQY
Copy link
Member Author

ErikQQY commented Sep 29, 2023

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:
old

SciMLBase 2.0.6:
new

@prbzrg
Copy link
Member

prbzrg commented Sep 29, 2023

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.

@ChrisRackauckas
Copy link
Member

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

@ChrisRackauckas ChrisRackauckas merged commit f2d7e98 into SciML:master Sep 29, 2023
31 of 32 checks passed
@ChrisRackauckas
Copy link
Member

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.

@ErikQQY ErikQQY deleted the qqy/refactor_sde branch September 30, 2023 10:57
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.

3 participants