Do not check for riemann_solver when running IGR#1530
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Code review findings from automated multi-angle analysis. Four issues found — two confirmed correctness gaps and two design concerns.
| igr = self.get("igr", "F") == "T" | ||
|
|
||
| if igr: | ||
| return |
There was a problem hiding this comment.
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
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Code review — 4 findings (2 confirmed correctness bugs, 2 design concerns). See inline comments.
| igr = self.get("igr", "F") == "T" | ||
|
|
||
| if igr: | ||
| return |
There was a problem hiding this comment.
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.
| igr = self.get("igr", "F") == "T" | ||
|
|
||
| if igr: | ||
| return |
There was a problem hiding this comment.
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.
| viscous = self.get("viscous", "F") == "T" | ||
| igr = self.get("igr", "F") == "T" | ||
|
|
||
| if igr: |
There was a problem hiding this comment.
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], ...)| 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" |
There was a problem hiding this comment.
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.
| igr = self.get("igr", "F") == "T" | ||
|
|
||
| if igr: | ||
| return |
There was a problem hiding this comment.
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.
| igr = self.get("igr", "F") == "T" | ||
|
|
||
| if igr: | ||
| return |
There was a problem hiding this comment.
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.
| viscous = self.get("viscous", "F") == "T" | ||
| igr = self.get("igr", "F") == "T" | ||
|
|
||
| if igr: |
There was a problem hiding this comment.
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], ...)
| 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" |
There was a problem hiding this comment.
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.
2 participants
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
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)claude-full-review— Claude full review via label