Skip to content

Fix/mhd cleaning speed #1235

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

Open
ChrisZYJ wants to merge 9 commits into
base: master
Choose a base branch
from
+232 −251
Open

Fix/mhd cleaning speed #1235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d51aaa7
Fix MHD hyperbolic cleaning using wrong sign for left fast magnetoson…
sbryngelson Feb 21, 2026
36bf826
Regenerate golden files for MHD tests
sbryngelson Feb 22, 2026
b90fc2c
fix hyper cleaning speed and update golden
ChrisZYJ Feb 22, 2026
d24ce52
generate missed golden
ChrisZYJ Feb 22, 2026
5b7ee95
Merge branch 'master' into fix/mhd-cleaning-speed
sbryngelson Feb 22, 2026
a502d7e
Merge branch 'master' into fix/mhd-cleaning-speed
sbryngelson Feb 23, 2026
5319d6c
Merge branch 'master' into fix/mhd-cleaning-speed
sbryngelson Feb 24, 2026
e553bbc
Merge branch 'master' into fix/mhd-cleaning-speed
sbryngelson Feb 26, 2026
8b37c79
Merge branch 'master' into fix/mhd-cleaning-speed
sbryngelson Mar 9, 2026
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
11 changes: 6 additions & 5 deletions src/simulation/m_riemann_solvers.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,11 +652,6 @@ contains
call s_compute_fast_magnetosonic_speed(rho_R, c_R, B%R, norm_dir, c_fast%R, H_R)
end if

if (hyper_cleaning) then ! mhd
c_fast%L = min(c_fast%L, -hyper_cleaning_speed)
c_fast%R = max(c_fast%R, hyper_cleaning_speed)
end if

if (viscous) then
if (chemistry) then
call compute_viscosity_and_inversion(T_L, Ys_L, T_R, Ys_R, Re_L(1), Re_R(1))
Expand Down Expand Up @@ -694,6 +689,12 @@ contains
s_R = max(vel_R(dir_idx(1)) + c_R, vel_L(dir_idx(1)) + c_L)
end if

if (hyper_cleaning) then
! Dedner GLM: (B_n, psi) subsystem has eigenvalues +/- c_h in the lab frame.
s_L = min(s_L, -hyper_cleaning_speed)
s_R = max(s_R, hyper_cleaning_speed)
Comment on lines +694 to +695
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

This assumes hyper_cleaning_speed is positive (since you use both -hyper_cleaning_speed and hyper_cleaning_speed as left/right bounds). If a user provides a negative value (there’s no validation in the toolchain currently), the bounds become inverted and the Riemann fan won’t be properly enclosed. Consider clamping with abs(hyper_cleaning_speed) here and/or adding a case-validator constraint that hyper_cleaning_speed > 0 whenever hyper_cleaning=T.

Suggested change
s_L = min(s_L, -hyper_cleaning_speed)
s_R = max(s_R, hyper_cleaning_speed)
s_L = min(s_L, -abs(hyper_cleaning_speed))
s_R = max(s_R, abs(hyper_cleaning_speed))

Copilot uses AI. Check for mistakes.
Comment on lines +692 to +695
Copy link
Contributor

@qodo-code-review qodo-code-review bot Mar 9, 2026

Choose a reason for hiding this comment

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

Action required

1. Negative c_h flips bounds 🐞 Bug ✓ Correctness

The new hyper_cleaning clamp in m_riemann_solvers.fpp assumes hyper_cleaning_speed is positive; if a
user supplies a negative c_h, s_L/s_R are clamped in the wrong direction and no longer bound the GLM
eigenvalues. The toolchain currently allows negative hyper_cleaning_speed because it has no range
constraint and check_mhd_simulation does not validate its sign.
Agent Prompt 
### Issue description
`hyper_cleaning_speed` (c_h) is used with an assumed positive sign in the new GLM wave-speed clamp (`s_L = min(s_L, -c_h)`, `s_R = max(s_R, c_h)`). Because the toolchain does not currently enforce `hyper_cleaning_speed > 0`, a negative user value will silently invert the intended clamping and produce incorrect Riemann bounds.

### Issue Context
- `hyper_cleaning_speed` is described/treated as a wave speed magnitude (c_h).
- Generic parameter validation only enforces constraints declared in the registry/constraints table; `hyper_cleaning_speed` has no min/max constraint.
- Physics validation (`check_mhd_simulation`) currently validates compatibility flags (mhd, n, wave_speeds) but does not validate the sign/range of `hyper_cleaning_speed`.

### Fix Focus Areas
- Add explicit physics validation when `hyper_cleaning` is enabled:
  - toolchain/mfc/case_validator.py[1026-1047]
- Add a generic numeric constraint so negative values are rejected early:
  - toolchain/mfc/params/definitions.py[594-633]
- (Optional but recommended) Add similar positivity checks for `hyper_cleaning_tau` because it is used as a divisor in damping terms.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +692 to +695
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 9, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Require a positive hyper_cleaning_speed before applying this clamp.

This assumes hyper_cleaning_speed > 0. If the parameter is unset or negative, Line 694/Line 695 silently produce the wrong GLM bounds instead of preserving ±c_h. Please enforce positivity in validation and defensively normalize here as well.

Suggested hardening 
-                                    s_L = min(s_L, -hyper_cleaning_speed)
-                                    s_R = max(s_R, hyper_cleaning_speed)
+                                    s_L = min(s_L, -abs(hyper_cleaning_speed))
+                                    s_R = max(s_R,  abs(hyper_cleaning_speed))

Based on learnings: Add physics validation checks in toolchain/mfc/case_validator.py via check_ methods for cross-parameter constraints beyond min/max.

end if

s_S = (pres_R - pres_L + rho_L*vel_L(dir_idx(1))* &
(s_L - vel_L(dir_idx(1))) - &
rho_R*vel_R(dir_idx(1))* &
Expand Down
131 changes: 63 additions & 68 deletions tests/B4DC99F9/golden-metadata.txt
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions tests/B4DC99F9/golden.txt
View file
Open in desktop

Large diffs are not rendered by default.

Loading
Loading