Remove the BARTH_JESPERSEN slope limiter - #2911
ManasBagul23 wants to merge 3 commits into
Conversation
LIMITER::BARTH_JESPERSEN used the Venkatakrishnan function with a machine-epsilon parameter, which is a smooth limiter and does not enforce the Barth-Jespersen bound. Use min(1, delta/proj), with 1 when the projection vanishes, so the reconstructed values do not exceed the neighbor extrema.
There was a problem hiding this comment.
🟡 Changes recommended
The epsilon guard can incorrectly return 1 for genuinely limiting nonzero projections.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Implements the bounded Barth–Jespersen limiter.
Changes:
- Replaces the Venkatakrishnan calculation with
min(1, delta / proj). - Handles zero projections by returning 1.
File summaries
| File | Review |
|---|---|
SU2_CFD/include/limiters/CLimiterDetails.hpp |
Critical: the epsilon guard can skip limiting for nonzero projections; use an exact-zero check. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| inline su2double limiterFunction(size_t, su2double proj, su2double delta) const | ||
| { | ||
| return LimiterHelpers<>::venkatFunction(proj, delta, eps2); | ||
| if (fabs(proj) <= eps) return 1.0; |
A projection smaller than machine epsilon is still a projection that may need limiting, only proj == 0 has nothing to limit.
|
@pcarruscag could you have a look at this one when you have time? It implements the Barth-Jespersen limiter as discussed in #1841. All CI checks pass except |
|
Hi @ManasBagul23 can you add a regression test for this? |
|
Let's just remove this limiter, it's terrible, nothing converges well with it. |
The option did not implement the Barth-Jespersen limiter (it used the Venkatakrishnan function), and a proper implementation converges poorly. Remove the option instead, as suggested in the review.
|
@pcarruscag done, the PR now removes @bigfooted with the limiter removed there is no code path left to test, so I did not add a regression test. Thanks for adding the label. |
Proposed Changes
Removes the
BARTH_JESPERSENslope limiter, as suggested in the review. The option did not implement the Barth-Jespersen limiter (it used the Venkatakrishnan function with a machine-epsilon parameter), and a propermin(1, delta/proj)implementation converges poorly.Removed:
LIMITER::BARTH_JESPERSENand its entry inLimiter_Map(option_structure.hpp)CLimiterDetails<LIMITER::BARTH_JESPERSEN>specialization and its case incomputeLimitersCConfiglimiter summary and the option in theconfig_template.cfglistA configuration that still uses it now stops with:
No TestCases use
BARTH_JESPERSEN. The residuals ofTestCases/euler/naca0012/inv_NACA0012_Roe.cfgwithVENKATAKRISHNANare unchanged. The limiter documentation on the website (Slope-Limiters-and-Shock-Resolution.md and three tutorial configs) still mentions the option, I will update it once this is merged.Related Work
Closes #1841, which proposed either implementing the limiter or removing the option.
PR Checklist
pre-commit run --allto format old commits.