-
Notifications
You must be signed in to change notification settings - Fork 109
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
Modify optimal CFL calculation for PERK2 integrator and its docstring #2123
Modify optimal CFL calculation for PERK2 integrator and its docstring #2123
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2123 +/- ##
==========================================
- Coverage 96.36% 96.36% -0.00%
==========================================
Files 477 477
Lines 37750 37752 +2
==========================================
+ Hits 36376 36377 +1
- Misses 1374 1375 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Can you please update the docstring to reflect the change? You also need to allow nothing
for dt_opt
, i.e. something like dt_opt::Union{Float64, Nothing}
in the definition of the struct
. GitHub doesn't let me to comment on these lines.
Done! Thank you for your suggestion! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also note in the parameters list of the docstring that nothing
is the default for dt_opt
and shortly explain what this means (especially that using the CFL callback relies on dt_opt
)?
src/time_integration/paired_explicit_runge_kutta/methods_PERK2.jl
Outdated
Show resolved
Hide resolved
Co-authored-by: Joshua Lampert <[email protected]>
Done! Thx for the suggestion :D |
Thanks you two! I think this PR alone is not breaking, right? |
Yes, I agree. This should not be breaking. Everything that worked before, should also work with this PR. |
src/time_integration/paired_explicit_runge_kutta/methods_PERK2.jl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
According to @JoshuaLampert 's recommendation in #2008. I implemented these minor modifications to
PairedExplicitRK2
Now, the user can use one of the constructors:
PairedExplicitRK2(num_stages, base_path_monomial_coeffs::AbstractString, dt_opt = nothing, bS = 1.0, cS = 0.5)
without providing
dt_opt
unless the user wants to use StepsizeCallback without specifying thecfl_number
by calling the functioncalculate_cfl(ode_algorithm::AbstractPairedExplicitRKSingle, ode)
A note regarding what packages the user needs to import in order to use it is added to the integrator's docstring.