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

Added FineRKN5 Algorithm #1948

Merged
merged 20 commits into from
Jun 7, 2023
Merged

Added FineRKN5 Algorithm #1948

merged 20 commits into from
Jun 7, 2023

Conversation

HenryLangner
Copy link
Contributor

Added the 5th order Nyström-Method presented by J. M. Fine in the article "Low Order Practical Runge-Kutta-Nyström Methods" (Also linked here: #677). The embedded method is not yet implemented however I am working on it.

The method passes the convergence tests. Using the harmonic oscillator, the error is plotted below for the solution u at t=3.0 for different timesteps.

errorFineRKN5

JuliaFormatter has been used.

CC @ranocha

@ranocha ranocha mentioned this pull request May 31, 2023
20 tasks
Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks a lot! This looks already quite good to me. Interpolants and error estimators can be added later in another PR.

Could you please add FineRKN5 to docs/src/dynamical/nystrom.md to include it in the docs.

src/algorithms.jl Outdated Show resolved Hide resolved
src/algorithms.jl Outdated Show resolved Hide resolved
src/algorithms.jl Outdated Show resolved Hide resolved
src/algorithms.jl Outdated Show resolved Hide resolved
@HenryLangner HenryLangner requested a review from ranocha June 1, 2023 10:53
@ranocha
Copy link
Member

ranocha commented Jun 1, 2023

Thanks! The PR looks good so far but convergence tests are failing?

@HenryLangner
Copy link
Contributor Author

The convergence tests failing is strange. The only things changed or added in this PR is related to the algorithm FineRKN5. Testing the convergence manually with the (in place and out of place) examples used in tests/algconvergence/partitioned_methods_tests.jl, using the code

using OrdinaryDiffEq, Test, RecursiveArrayTools, DiffEqDevTools, Statistics

u0 = fill(0.0, 2);
v0 = ones(2);
function f1_harmonic(dv, v, u, p, t)
    dv .= -u
end;
function f2_harmonic(du, v, u, p, t)
    du .= v
end;
function harmonic_analytic(y0, p, x)
    v0, u0 = y0.x
    ArrayPartition(-u0 * sin(x) + v0 * cos(x), u0 * cos(x) + v0 * sin(x))
end;
ff_harmonic = DynamicalODEFunction(f1_harmonic, f2_harmonic; analytic = harmonic_analytic);
prob = DynamicalODEProblem(ff_harmonic, v0, u0, (0.0, 5.0));

dts = 1.0 ./ 2.0 .^ (5:-1:0);
println("In Place")
sim = test_convergence(dts, prob, FineRKN5(), dense_errors = true);
println(@test sim.𝒪est[:l2]≈6 rtol=1e-1)
println(@test sim.𝒪est[:L2]≈4 rtol=1e-1)

println("Out of Place")

u0 = 0.0;
v0 = 1.0;
function f1_harmonic_nip(v, u, p, t)
    -u
end;
function f2_harmonic_nip(v, u, p, t)
    v
end;

ff_harmonic_nip = DynamicalODEFunction(f1_harmonic_nip, f2_harmonic_nip;
                                       analytic = harmonic_analytic);
prob = DynamicalODEProblem(ff_harmonic_nip, v0, u0, (0.0, 5.0));

sim = test_convergence(dts, prob, FineRKN5(), dense_errors = true);
println(@test sim.𝒪est[:l2]≈6 rtol=1e-1)
println(@test sim.𝒪est[:L2]≈4 rtol=1e-1)

grants
grafik.
Calling the Test results I get
grafik
for the In-Place test and
grafik
for the Out-of-Place test. Which shows that FineRKN5 converges fine. Am I missing something here?

I just checked the PR #1949 and it looks like it is failing the same tests with the same error message.

@ranocha
Copy link
Member

ranocha commented Jun 1, 2023

@ChrisRackauckas Is this a known issue? I can't debug it locally right now...

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good to me. The test failures are not related to this PR. May be fixed by merging current master into this PR?

Comment on lines 5255 to 5256
In case the ODE does not depend on the first derivative, consider using
[`Nystrom5VelocityIndependent`](@ref) to increase performance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't call out this specific method because it's not guaranteed to not change, so this information is non-local. Instead just say that this method requires that the acceleration equation needs to be independent of the velocity.

@ChrisRackauckas
Copy link
Member

@ChrisRackauckas Is this a known issue? I can't debug it locally right now...

yes it looks like it was an unrelated regression #1956

src/algorithms.jl Outdated Show resolved Hide resolved
@ChrisRackauckas ChrisRackauckas merged commit 1891a16 into SciML:master Jun 7, 2023
@ChrisRackauckas
Copy link
Member

Thanks!

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