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

Begin Rosenbrock refactor #2430

Merged
merged 61 commits into from
Sep 1, 2024
Merged

Conversation

ParamThakkar123
Copy link
Contributor

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.
solves a part of #233

@ParamThakkar123
Copy link
Contributor Author

image

@ChrisRackauckas @oscardssmith I am getting a error here saying RosenbrockCache has no field du1. This error occurs in derivative_utils.jl. I changed the line to dus so that it matches with the fields present in RosenbrockCache but that creates and error in NLNewtonCache.

Just had a question, how do I make a change so that this error gets solve and doesn't propagate or create errors in other caches ?

@ParamThakkar123
Copy link
Contributor Author

image

@ChrisRackauckas @oscardssmith I am getting a error here saying RosenbrockCache has no field du1. This error occurs in derivative_utils.jl. I changed the line to dus so that it matches with the fields present in RosenbrockCache but that creates and error in NLNewtonCache.

Just had a question, how do I make a change so that this error gets solve and doesn't propagate or create errors in other caches ?

I managed to solve the other ones. But needed your suggestion on this one

@ChrisRackauckas
Copy link
Member

du1 is used in the Jacobian calculation, so it's required for all methods that have a build_jac_config.

@ParamThakkar123
Copy link
Contributor Author

ParamThakkar123 commented Aug 25, 2024

du1 is used in the Jacobian calculation, so it's required for all methods that have a build_jac_config.

Should I be adding that explicitly as a field there ??

@ChrisRackauckas
Copy link
Member

yes

@oscardssmith
Copy link
Contributor

I've pushed a bunch of fixes to this. Specifically, du, du1 and du2 shouldn't have been a Vector, H should be a matrix, and the size of the loops should be based on the size of A rather than a fixed constant.

@ParamThakkar123
Copy link
Contributor Author

Got it. I think I made similar mistakes in tsit. I will make a fix there too.

@ChrisRackauckas
Copy link
Member

Got it. I think I made similar mistakes in tsit. I will make a fix there too.

No, as mentioned in other places, Tsit is completely different. It already has ExplicitRK. It's more about feature completeness of ExplicitRK and its performance.

@ParamThakkar123
Copy link
Contributor Author

Okay, @ChrisRackauckas @oscardssmith I will start working on other solvers then. Which solvers should be the next one ?? Though I started working on SDIRK and Tsit on my own but just wanted to know if you have any priorities with respect to solvers regarding which ones should be done first

@oscardssmith
Copy link
Contributor

stats look identical, and performance looks marginally better somehow? I think this is ready to merge.

function rober(u, p, t)
   y₁, y₂, y₃ = u
   k₁, k₂, k₃ = p
   [-k₁ * y₁ + k₃ * y₂ * y₃,
       k₁ * y₁ - k₃ * y₂ * y₃ - k₂ * y₂^2,
       k₂ * y₂^2]
end
prob_rober = ODEProblem(rober, [1.0, 0.0, 0.0], (0.0, 1e3), (0.04, 3e7, 1e4))

# PR
julia> @benchmark sol = solve(prob_rober, Rodas4())
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min … max):  190.246 μs … 127.780 ms  ┊ GC (min … max):  0.00% … 99.72%
 Time  (median):     220.651 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   281.783 μs ±   1.310 ms  ┊ GC (mean ± σ):  13.86% ±  8.56%

   ██▅▄▃▂▁▂▃▃▁                                                  ▂
  █████████████▇▇▆▇▆▅▆▅▄▃▄▁▃▁▄▄▃▁▃▄▁▃▄▃▄▄▁▅▄▄▄▅▅▄▆▆▅▄▆▅▄▆▇▆▅▆▆▆ █
  190 μs        Histogram: log(frequency) by time       1.02 ms <
# v 6.89
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min … max):  203.744 μs … 129.049 ms  ┊ GC (min … max):  0.00% … 99.74%
 Time  (median):     234.780 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   277.780 μs ±   1.306 ms  ┊ GC (mean ± σ):  12.14% ±  8.67%

      ▄▆██▇▆▅▄▃▂▂▁▁              ▁▂▁▁▁▁▂▁                       ▂
  ▆▆▆███████████████▇▆▇██▇▆▅▆▆▅▆██████████▇█▇▇▅▄▄▅▁▃▄▄▃▅▄▃▄▄▁▄▅ █
  204 μs        Histogram: log(frequency) by time        447 μs <

 Memory estimate: 452.92 KiB, allocs estimate: 5914.

@ParamThakkar123
Copy link
Contributor Author

@oscardssmith there's an undef error saying du is not defined in stiff_addsteps

@ParamThakkar123
Copy link
Contributor Author

Should I push a commit to fix it now

@oscardssmith
Copy link
Contributor

oscardssmith commented Aug 29, 2024

@ParamThakkar123 where was that error? I didn't see it...

Edit... Oh I do. Your fix is slightly wrong. updated fix incoming.

@ParamThakkar123
Copy link
Contributor Author

image

@oscardssmith here's a snapshot of the error. It can be seen in the buildkite tests inside integratorII_v1 tests

@oscardssmith
Copy link
Contributor

@ParamThakkar123 please stop pushing incorrect fixes. du is not supposed to be in the ConstantCache.

@ParamThakkar123
Copy link
Contributor Author

@oscardssmith okay sorry for that

@oscardssmith
Copy link
Contributor

@ChrisRackauckas good to merge?

@oscardssmith
Copy link
Contributor

ok, ready to merge?

@ChrisRackauckas ChrisRackauckas merged commit 1eef9db into SciML:master Sep 1, 2024
55 of 60 checks passed
@ChrisRackauckas
Copy link
Member

There's still a lot more to do of course with Rosenbrock, since there's now two generic caches (generic_rosenbrock + this one), and this only got the 4th order.

@ParamThakkar123
Copy link
Contributor Author

Yes @ChrisRackauckas . I am working on that part too. Will start with a new PR addressing this in an hour or two.

@oscardssmith oscardssmith changed the title Perform step refactor for rosenbrock Begin Rosenbrock refactor Oct 2, 2024
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