Skip to content

Do not check for riemann_solver when running IGR #1530

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.

Sign up for GitHub

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

Draft
Cowsreal wants to merge 1 commit into
base: master
Choose a base branch
from
+4 −0
Draft

Do not check for riemann_solver when running IGR #1530

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions toolchain/mfc/case_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,10 @@ def check_riemann_solver(self):
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"
Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if igr:
Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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], ...)

Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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], ...)

return
Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@sbryngelson sbryngelson Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


self.prohibit(riemann_solver is None, "riemann_solver must be specified (1=HLL, 2=HLLC, 4=HLLD, 5=Lax-Friedrichs)")
if riemann_solver is None:
Expand Down
Loading