muscl_eps by ChrisZYJ · Pull Request #1383 · MFlowCode/MFC

ChrisZYJ · 2026-04-28T11:39:33Z

Description

Add muscl_eps parameter to control MUSCL limiter slope-product threshold. Default preserves existing behavior;
muscl_eps = 0 gives textbook limiter behavior.

Type of change

New feature

Testing

All 30 existing MUSCL tests tests passed. Bit-identical default behavior as the old MUSCL code.
15 new muscl_eps=0 tests added (5 limiters x 3 dims)

Checklist

I added or updated tests for new behavior
I updated documentation if user-facing behavior changed
GPU results match CPU results
Tested on NVIDIA GPU or AMD GPU

AI code reviews

Reviews are not triggered automatically. To request a review, comment on the PR:

@coderabbitai review — incremental review (new changes only)
@coderabbitai full review — full review from scratch
/review — Qodo review
/improve — Qodo code suggestions
@claude full review — Claude full review (also triggers on PR open/reopen/ready)
Add label claude-full-review — Claude full review via label

coderabbitai · 2026-04-28T11:45:12Z

📝 Walkthrough

Walkthrough

This pull request introduces a new configurable muscl_eps parameter for MUSCL slope-limiter reconstruction. The parameter is declared globally in the simulation module, synchronized to GPU, and integrated into input file parsing. Default values are assigned based on limiter type during initialization (1e-9 for limiters 1-2, otherwise 1e-6). All five slope limiters (minmod, MC, Van Albada, Van Leer, SUPERBEE) are updated to use this shared threshold instead of previously hardcoded values. The parameter is registered in the toolchain with validation requiring non-negative values and extended test case generation to cover both default and muscl_eps=0 configurations. Supporting changes include SILO CMake build fixes and multiple new test golden data files.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'muscl_eps' is extremely vague and does not clearly describe the main change; it simply names the parameter without context about what the change accomplishes.	Replace with a more descriptive title such as 'Add muscl_eps parameter for MUSCL limiter slope-product threshold control' to clearly convey the feature and intent.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The pull request description is complete and follows the template structure with all required sections properly filled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/simulation/m_global_parameters.fpp (1)

865-867: Consider replacing literal thresholds with named constants.

Using named module constants for 1e-9_wp and 1e-6_wp would make intent clearer and reduce future drift risk.
tests/28D9A304/golden-metadata.txt (1)
1-157: Consider sanitizing machine-specific metadata in committed golden files.

This snapshot includes local absolute paths (/home/chris/...), a dirty git state, and host-specific hardware data, which can cause unnecessary churn and environment leakage in versioned fixtures. Prefer normalized/redacted fields for stable, portable metadata.
Example normalization approach
- This file was created on 2026-04-26 03:20:07.478463.
+ This file was created on <timestamp>.

- Git:        be1e6654c61a88ef82ca9fc539f2cb33f20af480 on MUSCL_fix (dirty)
+ Git:        <commit> on <branch> (<clean|dirty>)

- Fypp    : /home/chris/source/MFC_MUSCL_fix/build/venv/bin/fypp
+ Fypp    : <venv>/bin/fypp

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2a4094ac-d73e-4e34-8f39-8291d0068bc7

📥 Commits

Reviewing files that changed from the base of the PR and between 00aa603 and 1f681f2.

📒 Files selected for processing (39)

docs/documentation/case.md
src/simulation/m_global_parameters.fpp
src/simulation/m_muscl.fpp
src/simulation/m_start_up.fpp
tests/01DC0251/golden-metadata.txt
tests/01DC0251/golden.txt
tests/28D9A304/golden-metadata.txt
tests/28D9A304/golden.txt
tests/30035426/golden-metadata.txt
tests/30035426/golden.txt
tests/347964C3/golden-metadata.txt
tests/347964C3/golden.txt
tests/374C9B16/golden-metadata.txt
tests/374C9B16/golden.txt
tests/42848784/golden-metadata.txt
tests/42848784/golden.txt
tests/4F79391A/golden-metadata.txt
tests/4F79391A/golden.txt
tests/5EC347F2/golden-metadata.txt
tests/5EC347F2/golden.txt
tests/71258175/golden-metadata.txt
tests/71258175/golden.txt
tests/77292B06/golden-metadata.txt
tests/77292B06/golden.txt
tests/9BD657CB/golden-metadata.txt
tests/9BD657CB/golden.txt
tests/B46CEFF3/golden-metadata.txt
tests/B46CEFF3/golden.txt
tests/D68EDC50/golden-metadata.txt
tests/D68EDC50/golden.txt
tests/F207F0FE/golden-metadata.txt
tests/F207F0FE/golden.txt
tests/F4E54FC0/golden-metadata.txt
tests/F4E54FC0/golden.txt
toolchain/dependencies/CMakeLists.txt
toolchain/mfc/case_validator.py
toolchain/mfc/params/definitions.py
toolchain/mfc/params/descriptions.py
toolchain/mfc/test/cases.py

sbryngelson · 2026-04-28T13:48:03Z

you're mixing contributions in one pr

I'll open a new PR for that

codecov · 2026-04-28T13:48:57Z

Codecov Report

❌ Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.75%. Comparing base (be1e665) to head (1f681f2).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/simulation/m_global_parameters.fpp	60.00%	0 Missing and 2 partials ⚠️
src/simulation/m_muscl.fpp	33.33%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1383      +/-   ##
==========================================
- Coverage   64.76%   64.75%   -0.01%     
==========================================
  Files          71       71              
  Lines       18713    18718       +5     
  Branches     1549     1552       +3     
==========================================
+ Hits        12119    12121       +2     
  Misses       5638     5638              
- Partials      956      959       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sbryngelson · 2026-04-28T13:49:10Z

        #:endif

+        ! muscl_eps: use per-limiter defaults when user did not set it
+        if (muscl_eps == dflt_real) then


using == to compare real to real. i think we have f_approx_equal for this

Yeah sorry I introduced f_approx_equal a long time ago but forgot about it

sbryngelson · 2026-04-28T13:49:59Z

        namelist /user_inputs/ case_dir, run_time_info, m, n, p, dt, &
            t_step_start, t_step_stop, t_step_save, t_step_print, &
-            model_eqns, mpp_lim, time_stepper, weno_eps, &
+            model_eqns, mpp_lim, time_stepper, weno_eps, muscl_eps, &


2 vars -> 1 via reconst_eps which is either weno_eps or muscl_eps, depending on which method is used?

I feel that we should keep "weno_eps" for backwards compatibility to not break existing case files, so maybe we can use weno_eps for both, or have both weno_eps and muscl_eps. Personally I feel that muscl_eps for MSUCL is cleaner?

I think going to one, then updating all the case files at once via a sed replace (AI-friendly!) is fine.

MUSCL

1f681f2

coderabbitai Bot reviewed Apr 28, 2026

View reviewed changes

Comment thread toolchain/mfc/case_validator.py

sbryngelson reviewed Apr 28, 2026

View reviewed changes

sbryngelson marked this pull request as draft April 28, 2026 13:50

f_is_default and revert CMake changes

f5289ca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

muscl_eps#1383

muscl_eps#1383
ChrisZYJ wants to merge 2 commits intoMFlowCode:masterfrom
ChrisZYJ:MUSCL_fix

ChrisZYJ commented Apr 28, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Apr 28, 2026 •

edited

Loading

Walkthrough

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

sbryngelson Apr 28, 2026

Uh oh!

ChrisZYJ Apr 28, 2026

Uh oh!

codecov Bot commented Apr 28, 2026

Uh oh!

sbryngelson Apr 28, 2026

Uh oh!

ChrisZYJ Apr 28, 2026

Uh oh!

sbryngelson Apr 28, 2026

Uh oh!

ChrisZYJ Apr 28, 2026

Uh oh!

sbryngelson Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

ChrisZYJ commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Testing

Checklist

AI code reviews

Uh oh!

coderabbitai Bot commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbryngelson Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

ChrisZYJ Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 28, 2026

Codecov Report

Uh oh!

sbryngelson Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

ChrisZYJ Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

sbryngelson Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

ChrisZYJ Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

sbryngelson Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

ChrisZYJ commented Apr 28, 2026 •

edited

Loading

coderabbitai Bot commented Apr 28, 2026 •

edited

Loading