-
Notifications
You must be signed in to change notification settings - Fork 112
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!
* make NLsolve a weapdep * fmt * add NEWS.md but TBD on the ref pull request * add comments and adjustment on solve_a_unknown * modular implementation with init, step!, and solve_step! * fmt * add test * adda body of p3 constructor test * changes according to test and correct variable names * only check the values of a_matrix from second row to end * adjust the the constructor of path coefficient and its test * adjust the test and add a seed to the randomized initial guess for reproducibility * add NLsolve as a dependency for testing * Update ext/TrixiNLsolveExt.jl Co-authored-by: Daniel Doehring <[email protected]> * Update ext/TrixiNLsolveExt.jl Co-authored-by: Daniel Doehring <[email protected]> * Update ext/TrixiNLsolveExt.jl Co-authored-by: Daniel Doehring <[email protected]> * Update ext/TrixiNLsolveExt.jl Co-authored-by: Daniel Doehring <[email protected]> * Update ext/TrixiNLsolveExt.jl Co-authored-by: Daniel Doehring <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * optimize the loop for step! by moving the condition outside * fmt * more type generic * change some names * update test * Correcting steps! * Apply suggestions from code review Co-authored-by: Daniel Doehring <[email protected]> * Update examples/structured_1d_dgsem/elixir_burgers_perk3.jl Co-authored-by: Daniel Doehring <[email protected]> * Update examples/structured_1d_dgsem/elixir_burgers_perk3.jl Co-authored-by: Daniel Doehring <[email protected]> * add docstring about dt_opt * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * merge k_higher in the last stage to a bigger loop * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * change solve_step! to solve! * Correct the logic of step! * deprecation * Optimize K_S1 away * fmt * remove dt_opt as an attribute of PERK3 * change the objective function to match the number of equations * fmt * minor comment fix * delete some stuff left from random * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * minor adjustments * minor change to the comment * add proper comment and bring seed back * update test values * fmt * change the keyword according to the error in the test pipeline and edit some values to match the test pipeline * remove unused import * fix test values in misc * add max iteration * Update ext/TrixiNLsolveExt.jl Co-authored-by: Daniel Doehring <[email protected]> * Apply suggestions from code review * remove the allocating part of is_sol_valid * removing dt_opt and update test values * Update NEWS.md Co-authored-by: Daniel Doehring <[email protected]> * update cfl number for the simulation * Update examples/structured_1d_dgsem/elixir_burgers_perk3.jl Co-authored-by: Daniel Doehring <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * change from a_stages_stages.txt to a_stages.txt * fixed step size should work with save solution now * Update examples/structured_1d_dgsem/elixir_burgers_perk3.jl Co-authored-by: Daniel Doehring <[email protected]> * add save solution to the example * update test to be compatible with save_solution * move comment regarding seed upwards * Revert "Correct the logic of step!" only the part that meddles with methods_PERK2 * correct methods_PERK3 * move is_sol_valid closer to the for loop * fmt * Revert some random changes in other test unit * add tolerance to the test * modify functions so that they are also compatible with PERK3 * change function's name to be more descriptive * change function's name to be more descriptive in all files * Revert irrelevent change in TrixiConvexECOSExt.jl * add PR number to NEWS.md * fmt * change from using Random to StableRNGs * fix the value in unit test * remove prints * minor comment correction * attempt to fix the error at fixed time step * add the missing clause to test set * adjust allocation values in test of perk3 * update test value * move objective function to the extension * minor fix with compute_c_coeffs * remove explicit import of solve_a_unknown from line 18 * Apply suggestions from code review * document why additional packages are loaded * correct docstring * use Float32 * Update ext/TrixiNLsolveExt.jl Co-authored-by: Hendrik Ranocha <[email protected]> * Update ext/TrixiNLsolveExt.jl Co-authored-by: Hendrik Ranocha <[email protected]> * Update ext/TrixiNLsolveExt.jl Co-authored-by: Hendrik Ranocha <[email protected]> * change some Flot32 back to the way they originally were * add line that get the type that a_unknown should be * due to some the change of type, print out some values of a_matrix that changes slighly from the original value (error value in other tests still remain the same) * update test values * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Joshua Lampert <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Joshua Lampert <[email protected]> * Apply suggestions from code review Co-authored-by: Joshua Lampert <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Joshua Lampert <[email protected]> * allocate c_eq once per solve_a_unknown is called * Update ext/TrixiNLsolveExt.jl Co-authored-by: Joshua Lampert <[email protected]> * Update ext/TrixiNLsolveExt.jl Co-authored-by: Joshua Lampert <[email protected]> * Update ext/TrixiNLsolveExt.jl Co-authored-by: Joshua Lampert <[email protected]> * minor fix regarding recent changes witjh c_eq * adjust a constructor to get num stages from reading the files directly * Revert "adjust a constructor to get num stages from reading the files directly" since it is breaking * Update TrixiNLsolveExt.jl to use forward autodiff in solve_a_butcher_coeffs_unknown! function * Apply suggestions from code review * Slight modifications a values * add cfl number calculation for PERK3 * update CI values * remove the example without cfl calculation * Apply suggestions from code review Co-authored-by: Daniel Doehring <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Daniel Doehring <[email protected]> * add DOI to a reference in TrixiNLsolveExt * update the test of perk3 * Update examples/structured_1d_dgsem/elixir_burgers_perk3.jl * Update examples/structured_1d_dgsem/elixir_burgers_perk3.jl Co-authored-by: Joshua Lampert <[email protected]> * Update examples/structured_1d_dgsem/elixir_burgers_perk3.jl Co-authored-by: Joshua Lampert <[email protected]> * add to docstring packages needed in order to use PERK3 * update NEWS.md * changes according to #2026 * add comments to the test without stepsize callback * Update ext/TrixiNLsolveExt.jl Co-authored-by: Joshua Lampert <[email protected]> * fmt * slight modification according to #2123 * Update test/test_unit.jl Co-authored-by: Joshua Lampert <[email protected]> * Update src/time_integration/paired_explicit_runge_kutta/methods_PERK3.jl Co-authored-by: Joshua Lampert <[email protected]> * fmt + minor correction * update NEWS.md * make functions more general for PERK * remove base.resize for perk3 * put all the general functions of PERK into paired_explicit_runge_kutta.jl * Update src/time_integration/paired_explicit_runge_kutta/paired_explicit_runge_kutta.jl Co-authored-by: Joshua Lampert <[email protected]> * modify some functions to be useable for both multi and single scheme * fmt * ensure type consistency in compute_c_coeffs * print out the full a_matrix * print out the new a_matrix from the latest change * remove the false line of PERK construction * fix the test values * move using statements to src/Trixi.jl --------- Co-authored-by: Daniel Doehring <[email protected]> Co-authored-by: Hendrik Ranocha <[email protected]> Co-authored-by: Joshua Lampert <[email protected]>
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.