-
Notifications
You must be signed in to change notification settings - Fork 114
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
Add one-sided local subcell IDP limiting for non-linear variables #1792
Add one-sided local subcell IDP limiting for non-linear variables #1792
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 #1792 +/- ##
==========================================
- Coverage 96.11% 96.10% -0.01%
==========================================
Files 451 451
Lines 36261 36414 +153
==========================================
+ Hits 34850 34993 +143
- Misses 1411 1421 +10
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.
Almost there 😊
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.
LGTM!
All failures seem to be Codecov related, but we still cannot see code coverage 😢 |
If it now fails again, I will rerun the failed tests overnight. That works most of the time. |
@sloede All tests have been run through now. The codecov tests still slightly fail, mostly due to not tested line within the saving deviations part of the BoundsCheckCallback. |
It seems to me that I/O for one-sided or two-sided limiters is not covered in the tests. If I were you, I'd make sure to have this part executed at least in one test to ensure it will continue to work even if you decide to modify some variables somewhere else. But I'll leave the decision up to you on how to proceed. |
I already included a two sided limiter I/O test in #1824. So, I think it would merge this PR as it is and handle the one sided I/O in the other PR (or a completely new one). |
Are you sure? On Codecov, I am seeing this for for But if that's ok, we can still merge right away 😊 |
Oh, you're right. I just added the test for So, I would go for it and merge it 🚀 |
The positivity limiting for non-linear variables was merged (PR) and therefore the Newton-bisection method is now included to
main
.So now, the Newton method can easily be used for a local one-sided limiting of nonlinear variables.
General variables, e.g.
entropy_guermond_etal
orentropy_math
, can be passed combined with a specification of the wanted bound, i.e. passmin
for a lower bound andmax
for upper bounds.Example: Pass the following parameter within the limiter
Additional, in this PR I renamed
local_minmax
tolocal_twosided
to be consistent withlocal_onesided
.