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

Some methods are in the wrong sub-package #2373

Closed
ranocha opened this issue Aug 12, 2024 · 11 comments · Fixed by #2408
Closed

Some methods are in the wrong sub-package #2373

ranocha opened this issue Aug 12, 2024 · 11 comments · Fixed by #2408

Comments

@ranocha
Copy link
Member

ranocha commented Aug 12, 2024

  1. SHLDDRK_2N and SHLDDRK52 should be moved from OrdinaryDiffEqSSPRK.jl to OrdinaryDiffEqLowStorageRK.jl since they are not SSP
  2. KYK2014DGSSPRK_3S2 should be moved from OrdinaryDiffEqLowStorageRK.jl to OrdinaryDiffEqSSPRK.jl since the other SSP methods are there, even if they are low storage (which most of them are)
  3. The docstring of ssp_coefficient should move from OrdinaryDiffEqLowStorageRK.jl to OrdinaryDiffEqSSPRK.jl or OrdinaryDiffEqCore.jl

Details

SSPRK33, SHLDDRK_2N, KYKSSPRK42, SHLDDRK52

SHLDDRK_2N and SHLDDRK52 are not SSP methods but low-storage methods. There is also no ssp_coefficient defined for them, see

ssp_coefficient(alg::SSPRK53_2N1) = 2.18
ssp_coefficient(alg::SSPRK53_2N2) = 2.148
ssp_coefficient(alg::SSPRK53) = 2.65
ssp_coefficient(alg::SSPRK53_H) = 2.65
ssp_coefficient(alg::SSPRK63) = 3.518
ssp_coefficient(alg::SSPRK73) = 4.2879
ssp_coefficient(alg::SSPRK83) = 5.107
ssp_coefficient(alg::SSPRK43) = 2
ssp_coefficient(alg::SSPRK432) = 2
ssp_coefficient(alg::SSPRK932) = 6
ssp_coefficient(alg::SSPRK54) = 1.508
ssp_coefficient(alg::SSPRK104) = 6
ssp_coefficient(alg::SSPRK33) = 1
ssp_coefficient(alg::SSPRK22) = 1
ssp_coefficient(alg::SSPRKMSVS32) = 0.5
ssp_coefficient(alg::SSPRKMSVS43) = 0.33
ssp_coefficient(alg::KYKSSPRK42) = 2.459

KYK2014DGSSPRK_3S2 is an SSP method, see also

"""
ssp_coefficient(alg)
Return the SSP coefficient of the ODE algorithm `alg`. If one time step of size
`dt` with `alg` can be written as a convex combination of explicit Euler steps
with step sizes `cᵢ * dt`, the SSP coefficient is the minimal value of `1/cᵢ`.
# Examples
```julia-repl
julia> ssp_coefficient(SSPRK104())
6
```
"""
ssp_coefficient(alg::KYK2014DGSSPRK_3S2) = 0.8417

This was referenced Aug 12, 2024
@ChrisRackauckas
Copy link
Member

@ParamThakkar123 can you take a look at this?

@ParamThakkar123
Copy link
Contributor

Yes I will take this. I will make the change

@ranocha
Copy link
Member Author

ranocha commented Aug 12, 2024

Thanks!

ParamThakkar123 added a commit to ParamThakkar123/OrdinaryDiffEq.jl that referenced this issue Aug 12, 2024
@ParamThakkar123
Copy link
Contributor

@ranocha the fix has been incorporated in my recent PR #2320

@oscardssmith
Copy link
Contributor

IMO it relaly should be a separate PR to make review easier (especially because I expect this change to merge sooner).

ParamThakkar123 added a commit to ParamThakkar123/OrdinaryDiffEq.jl that referenced this issue Aug 13, 2024
ParamThakkar123 added a commit to ParamThakkar123/OrdinaryDiffEq.jl that referenced this issue Aug 13, 2024
@ChrisRackauckas
Copy link
Member

Did these fixes get a separate PR?

@ParamThakkar123
Copy link
Contributor

Did these fixes get a separate PR?

Actually I first did this in the same PR #2320. But then on @oscardssmith suggestion, I reverted it and will start a new one soon.

ParamThakkar123 added a commit to ParamThakkar123/OrdinaryDiffEq.jl that referenced this issue Aug 22, 2024
ParamThakkar123 added a commit to ParamThakkar123/OrdinaryDiffEq.jl that referenced this issue Aug 22, 2024
ParamThakkar123 added a commit to ParamThakkar123/OrdinaryDiffEq.jl that referenced this issue Aug 22, 2024
@ParamThakkar123
Copy link
Contributor

ParamThakkar123 commented Aug 22, 2024

@ranocha should the cache also be modified like in SHLDDRK_2N uses SSPRKMutableCache

like here :

image

@ChrisRackauckas
Copy link
Member

yes fix the abstract types.

@ParamThakkar123
Copy link
Contributor

yes fix the abstract types.

Should I replace SSPRKMutableCache directly to LowStorageRKCache ?? Or there would be some other changes to make

@ChrisRackauckas
Copy link
Member

Both subpackages have an abstract type for the caches of their group. Just apply the one for the destination.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants