-
Notifications
You must be signed in to change notification settings - Fork 126
Add unstable_check
for custom SSP method
#2445
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
base: main
Are you sure you want to change the base?
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 #2445 +/- ##
==========================================
+ Coverage 94.86% 96.21% +1.35%
==========================================
Files 506 509 +3
Lines 41952 42116 +164
==========================================
+ Hits 39795 40519 +724
+ Misses 2157 1597 -560
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Is used correctly in the custom time integrators, i.e., over the ODE.jl implementation?
In other words, no explicit example needed?
I'm not really sure what you mean. Thanks for the reference. I didn't know that |
I'm also thinking about adding this to the other custom time integration methods. What do you think? |
It would make sense to unify this. However, the next question is how "correct" it should be.
|
Hm yeah so I also though about this at some point (see the PR) but disregarded this as I did ultimately not care too much if we would take a fistful more timesteps before breaking down. |
@@ -307,7 +307,7 @@ export PositivityPreservingLimiterZhangShu, EntropyBoundedLimiter | |||
export trixi_include, examples_dir, get_examples, default_example, | |||
default_example_unstructured, ode_default_options | |||
|
|||
export ode_norm, ode_unstable_check | |||
export ode_norm, ode_unstable_check, unstable_check |
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.
If we export it, it becomes part of our public API (although it is not documented...). However, I do not think this is a good idea right now since it can be easily confused with ode_unstable_check
. I see two options right now:
- Do something specific for your subcell limiter ecosystem
- Make it general so that it works nicely with the other parts
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.
Okay, that's true.
I'm completely fine with the first option. But how would that look like in your opinion? Rename it to something like unstable_check_SSPRK
, have an optional parameter unstable_check = nothing
for solve()
of the custom time integrators and add unstable_check = unstable_check_SSRK
to every elixir, where it's needed?
Or really removing the parameter unstable_check
from the solve function and hardcode it into the SSPRK method?
This PR adds an instability check for the custom SSPRK time integration method.
Currently, when running an unstable simulation (e.g. sedov blast (
elixir_euler_sedov_blast_wave_sc_subcell.jl
without any limiting) the simulation is already unstable after the second time stepNevertheless, the simulation continues to time step 5:
When using the SSPRK33 method of OrdinaryDiffEqSSPRK, we get the following warning after the second time step and the simulation terminates afterward.
from here.
I want to mimic that with this simple instability check.