Do not check for riemann_solver when running IGR by Cowsreal · Pull Request #1530 · MFlowCode/MFC

Cowsreal · 2026-06-02T17:55:37Z

Description

When running IGR, the riemann_solver parameter has no functional code purpose as IGR is self contained entirely inside m_igr.fpp. Currently running IGR without writing the riemann_solver parameter results in a case validation error.

Type of change

Bug fix

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

codecov · 2026-06-02T20:14:45Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.64%. Comparing base (9e862e8) to head (13bcc78).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1530   +/-   ##
=======================================
  Coverage   60.64%   60.64%           
=======================================
  Files          73       73           
  Lines       20213    20213           
  Branches     2936     2936           
=======================================
  Hits        12259    12259           
  Misses       5966     5966           
  Partials     1988     1988

☔ 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

Code review findings from automated multi-angle analysis. Four issues found — two confirmed correctness gaps and two design concerns.

sbryngelson · 2026-06-02T20:57:51Z

+        igr = self.get("igr", "F") == "T"
+
+        if igr:
+            return


Bug: low_Mach range check is bypassed for all IGR cases

The early return skips the sole low_Mach not in [0, 1, 2] range check (line 686). check_igr_simulation() has no compensating validation for low_Mach — a user setting igr=T and low_Mach=5 passes Python validation silently. Confirmed: this is the only place in the codebase where the low_Mach range is validated.

sbryngelson

Code review — 4 findings (2 confirmed correctness bugs, 2 design concerns). See inline comments.

sbryngelson · 2026-06-02T20:58:12Z

+        igr = self.get("igr", "F") == "T"
+
+        if igr:
+            return


Bug: low_Mach range check is bypassed for all IGR cases

The early return skips the sole low_Mach not in [0, 1, 2] range check (line 686). check_igr_simulation() has no compensating validation for low_Mach, so a user setting igr=T and low_Mach=5 silently passes Python validation with no error. This is the only place in the codebase that validates the low_Mach range.

sbryngelson · 2026-06-02T20:58:12Z

+        igr = self.get("igr", "F") == "T"
+
+        if igr:
+            return


Bug: riemann_solver out-of-range values are not caught when igr=T

The early return also skips the riemann_solver not in [1, 2, 4, 5] check (line 680). No other check_* method provides a general range-validity guard. A user who accidentally sets igr=T alongside riemann_solver=3 (typo or confusion) will get no diagnostic.

sbryngelson · 2026-06-02T20:58:12Z

        viscous = self.get("viscous", "F") == "T"
+        igr = self.get("igr", "F") == "T"
+
+        if igr:


Design: early return is broader than needed

The fix's intent is to allow riemann_solver to be unset when IGR is active, but the blanket return also silently skips low_Mach, wave_speeds, and avg_state range checks that have nothing to do with whether riemann_solver is required. A targeted fix wraps only the riemann_solver-dependent checks:

if not igr: self.prohibit(riemann_solver is None, "riemann_solver must be specified ...") if riemann_solver is None: return self.prohibit(riemann_solver not in [1, 2, 4, 5], ...) # ... other riemann_solver-dependent checks ... # These run unconditionally: self.prohibit(low_Mach not in [0, 1, 2], "low_Mach must be 0, 1, or 2") self.prohibit(wave_speeds is not None and wave_speeds not in [1, 2], ...) self.prohibit(avg_state is not None and avg_state not in [1, 2], ...)

sbryngelson · 2026-06-02T20:58:12Z

        low_Mach = self.get("low_Mach", 0)
        cyl_coord = self.get("cyl_coord", "F") == "T"
        viscous = self.get("viscous", "F") == "T"
+        igr = self.get("igr", "F") == "T"


Design: IGR exemption lives in the wrong function

Every other IGR incompatibility is documented in check_igr_simulation() with an explicit message (15+ prohibit() calls for ib, bubbles, MHD, cyl_coord, etc.). This exemption is invisible from there — a developer auditing check_igr_simulation() for IGR's full contract will miss that riemann_solver is silently waived.

Consider adding to check_igr_simulation():

riemann_solver = self.get("riemann_solver") self.warn(riemann_solver is not None, "riemann_solver is set but ignored by IGR")

and then conditioning the prohibit(riemann_solver is None, ...) in this function on not igr rather than using a full early return.

sbryngelson · 2026-06-02T20:58:38Z

+        igr = self.get("igr", "F") == "T"
+
+        if igr:
+            return


Bug: low_Mach range check is bypassed for all IGR cases

The early return skips the sole low_Mach not in [0, 1, 2] range check (line 686). check_igr_simulation() has no compensating validation for low_Mach, so a user setting igr=T and low_Mach=5 silently passes Python validation with no error. This is the only place in the codebase that validates the low_Mach range.

sbryngelson · 2026-06-02T20:58:46Z

+        igr = self.get("igr", "F") == "T"
+
+        if igr:
+            return


Bug: invalid riemann_solver values not caught when igr=T

The early return also skips the riemann_solver not in [1, 2, 4, 5] check (line 680). No other check_* method provides a general range-validity guard. A user who accidentally sets igr=T alongside riemann_solver=3 (typo or confusion) will get no diagnostic — the bad value passes validation silently.

sbryngelson · 2026-06-02T20:58:56Z

        viscous = self.get("viscous", "F") == "T"
+        igr = self.get("igr", "F") == "T"
+
+        if igr:


Design: early return is broader than the fix needs to be

The stated goal is to allow riemann_solver to be unset when IGR is active. But this blanket return also silently skips low_Mach, wave_speeds, and avg_state range checks that are unrelated to riemann_solver being required. A targeted fix wraps only the riemann_solver-dependent checks:

if not igr: self.prohibit(riemann_solver is None, "riemann_solver must be specified ...") if riemann_solver is None: return self.prohibit(riemann_solver not in [1, 2, 4, 5], ...) # ... other riemann_solver-dependent checks ... # These run unconditionally: self.prohibit(low_Mach not in [0, 1, 2], "low_Mach must be 0, 1, or 2") self.prohibit(wave_speeds is not None and wave_speeds not in [1, 2], ...) self.prohibit(avg_state is not None and avg_state not in [1, 2], ...)

sbryngelson · 2026-06-02T20:59:05Z

        low_Mach = self.get("low_Mach", 0)
        cyl_coord = self.get("cyl_coord", "F") == "T"
        viscous = self.get("viscous", "F") == "T"
+        igr = self.get("igr", "F") == "T"


Design: IGR exemption should live in check_igr_simulation(), not here

Every other IGR incompatibility is documented in check_igr_simulation() with an explicit message (15+ prohibit() calls covering ib, bubbles, MHD, cyl_coord, etc.). A developer auditing that function for IGR's full constraint set will not see that riemann_solver is silently waived here.

Consider adding to check_igr_simulation():

riemann_solver = self.get("riemann_solver") self.warn(riemann_solver is not None, "riemann_solver is set but ignored by IGR")

and conditioning the prohibit(riemann_solver is None, ...) in check_riemann_solver() on not igr rather than using a blanket early return.

Remove riemann_solver check when running IGR

13bcc78

Cowsreal requested a review from sbryngelson as a code owner June 2, 2026 17:55

sbryngelson reviewed Jun 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not check for riemann_solver when running IGR#1530

Do not check for riemann_solver when running IGR#1530
Cowsreal wants to merge 1 commit into
MFlowCode:masterfrom
Cowsreal:minorChanges2

Cowsreal commented Jun 2, 2026

Uh oh!

codecov Bot commented Jun 2, 2026

Uh oh!

sbryngelson left a comment

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

sbryngelson left a comment

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

sbryngelson Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

Cowsreal commented Jun 2, 2026

Description

Type of change

AI code reviews

Uh oh!

codecov Bot commented Jun 2, 2026

Codecov Report

Uh oh!

sbryngelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbryngelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants