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

fix out-of-place version of FineRKN5 #1973

Merged
merged 17 commits into from
Jun 20, 2023
Merged

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Jun 19, 2023

I found some typos in out-of-place version of FineRKN5() and fixed them. I also added some tests checking that the in-place and out-of-place versions of some Runge-Kutta-Nyström methods. Some of these are currently broken, in particular with adaptive time stepping. I propose to open an issue and check them later.
I also fixed the reported number of function evaluations of some methods and added some checks for them.

CC @HenryLangner

@ranocha ranocha requested a review from ChrisRackauckas June 19, 2023 06:31
@ranocha ranocha added the bug label Jun 19, 2023
Copy link
Contributor

@HenryLangner HenryLangner left a comment

Choose a reason for hiding this comment

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

Something was off with the "out-of-place" version of FineRKN5. I missed these mistakes, thank you for fixing them!

I will add convergence test for the damped_oscillator in another PR.

src/perform_step/rkn_perform_step.jl Outdated Show resolved Hide resolved
@ranocha
Copy link
Member Author

ranocha commented Jun 19, 2023

I don't think the failing downstream tests are related to this PR - but they fail badly, e.g.,
https://github.com/SciML/OrdinaryDiffEq.jl/actions/runs/5309278846/jobs/9609731280?pr=1973#step:6:1413

signal (4): Illegal instruction
in expression starting at /home/runner/work/OrdinaryDiffEq.jl/OrdinaryDiffEq.jl/downstream/test/sde_neural.jl:13
_pullback at /home/runner/work/OrdinaryDiffEq.jl/OrdinaryDiffEq.jl/downstream/test/sde_neural.jl:104 [inlined]
_pullback at /home/runner/.julia/packages/Zygote/JeHtr/src/compiler/interface2.jl:0
_pullback at /home/runner/work/OrdinaryDiffEq.jl/OrdinaryDiffEq.jl/downstream/test/sde_neural.jl:133 [inlined]
_pullback at /home/runner/.julia/packages/Zygote/JeHtr/src/compiler/interface2.jl:0

@ranocha
Copy link
Member Author

ranocha commented Jun 20, 2023

@ChrisRackauckas This should be good, I think. @HenryLangner can update his recent PRs #1975 and #1976 with additional tests once this PR is merged.

@ChrisRackauckas
Copy link
Member

Was there a necessary order here? I merged the one I saw that looked good to go, so I think this just needs a quick merge resolution to be merged next? #1975 then has a few conflicts so it can be merged shortly after.

@ranocha
Copy link
Member Author

ranocha commented Jun 20, 2023

It was messed up a bit since @HenryLangner pulled the changes I made here into his PR. But it's fine, I fixed the merge conflicts here. I propose to merge this first and let @HenryLangner resolve the merge conflicts in #1975 afterwards, updating also the new tests added here with the new algorithm.

@HenryLangner
Copy link
Contributor

It was messed up a bit since @HenryLangner pulled the changes I made here into his PR. But it's fine, I fixed the merge conflicts here. I propose to merge this first and let @HenryLangner resolve the merge conflicts in #1975 afterwards, updating also the new tests added here with the new algorithm.

Sounds good. Once this PR is merged I will update my PR #1975, resolve merge conflicts and update the new tests.

@ChrisRackauckas ChrisRackauckas merged commit 3d84dcb into SciML:master Jun 20, 2023
@ranocha ranocha deleted the hr/FineRKN5 branch June 21, 2023 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants