Conversation
…ic speed min(c_fast%L, -hyper_cleaning_speed) forces c_fast%L negative, but fast magnetosonic speeds should be positive. Changed to max() to match the right-state treatment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update golden files for hyper_cleaning and mhd_rotor tests to reflect the corrected min/max sign for the left-state fast magnetosonic speed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
codeant-ai
bot
added
the
size:L
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
toolchain/mfc/case_validator.py (2)
1036-1037: Considerwave_speeds != 1to align with the error message and guard against future additions.The condition
wave_speeds == 2and the message"MHD requires wave_speeds = 1"are currently consistent (valid values are only 1 or 2), but expressing the constraint as!= 1better matches the stated requirement and stays correct if newwave_speedsvalues are ever introduced.♻️ Suggested change
- self.prohibit(mhd and wave_speeds is not None and wave_speeds == 2, + self.prohibit(mhd and wave_speeds is not None and wave_speeds != 1, "MHD requires wave_speeds = 1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/case_validator.py` around lines 1036 - 1037, The check in the case validation currently prohibits only wave_speeds == 2 when MHD is enabled; update the condition in the prohibit call (the one using self.prohibit with mhd and wave_speeds) to reject any value other than 1 (i.e., use wave_speeds is not None and wave_speeds != 1) so the error message "MHD requires wave_speeds = 1" stays correct and remains future-proof; keep the same error string and the same call site (the self.prohibit line referencing mhd and wave_speeds).
186-193: UpdatePHYSICS_DOCSto document the newwave_speeds = 1MHD constraint.The
check_mhdentry's explanation does not reflect the new simulation-specific requirement, andcheck_mhd_simulationhas noPHYSICS_DOCSentry at all. Per the contributing guidelines, physics constraints added to validator methods should be reflected inPHYSICS_DOCSso the auto-generated documentation stays accurate.Suggested addition to
PHYSICS_DOCS:"check_mhd_simulation": { "title": "MHD Simulation Constraints", "category": "Feature Compatibility", "explanation": ( "MHD simulations require riemann_solver = 1 (HLL) or 4 (HLLD). " "wave_speeds must be 1 (Roe averages) — wave_speeds = 2 is incompatible " "with the hyperbolic GLM cleaning speed treatment. " "HLLD is not available for RMHD. Hyperbolic cleaning requires 2D or 3D." ), },Based on learnings: "When adding a
check_method tocase_validator.py, document its physics in thePHYSICS_DOCSdict with title, category, explanation, and optional math/references/exceptions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/case_validator.py` around lines 186 - 193, PHYSICS_DOCS is missing documentation for the new wave_speeds constraint: update the "check_mhd" explanation to mention the new requirement that wave_speeds = 1 for MHD and add a new "check_mhd_simulation" entry in PHYSICS_DOCS describing riemann_solver = 1 (HLL) or 4 (HLLD), wave_speeds must be 1 (Roe averages) and that wave_speeds = 2 is incompatible with hyperbolic GLM cleaning, plus notes that HLLD is unavailable for RMHD and hyperbolic cleaning requires 2D/3D; use the PHYSICS_DOCS dict and reference the validator methods check_mhd and check_mhd_simulation so the auto-generated docs reflect the new constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/EC8F87D9/golden-metadata.txt`:
- Around line 1-7: The golden file was generated from a dirty workspace on
branch fix/mhd-cleaning-speed for test EC8F87D9; confirm whether the uncommitted
changes used to generate tests/golden-metadata.txt actually match the committed
fix by comparing the working tree to the commit (e.g., git status/diff against
commit 36bf82625f0ad02f42ceeb2fe0fba0eda80fab9f). If they differ, regenerate the
golden using a clean checkout of the intended commit and commit that golden (or
include the missing changes in the branch); if they match, update the metadata
to reflect a clean generation (remove “dirty” note or update the recorded Git
hash to the final commit) and leave the 1e-3 tolerance as-is since it is
intentionally loose for cross‑platform FP variability.
---
Duplicate comments:
In `@tests/B4DC99F9/golden-metadata.txt`:
- Line 7: The golden-metadata.txt for test "mhd_rotor" contains non-portable,
dirty-workspace and machine-specific info (commit shows as dirty and host
LAPTOP-OMEN); regenerate the golden metadata from a clean repo state (no
uncommitted changes) or CI runner so the commit hash is clean and
remove/normalize host-specific fields, then update the test's
golden-metadata.txt to the deterministic values (clean commit id and neutral
host/machine fields) so the test no longer depends on a local dirty workspace or
personal laptop.
---
Nitpick comments:
In `@toolchain/mfc/case_validator.py`:
- Around line 1036-1037: The check in the case validation currently prohibits
only wave_speeds == 2 when MHD is enabled; update the condition in the prohibit
call (the one using self.prohibit with mhd and wave_speeds) to reject any value
other than 1 (i.e., use wave_speeds is not None and wave_speeds != 1) so the
error message "MHD requires wave_speeds = 1" stays correct and remains
future-proof; keep the same error string and the same call site (the
self.prohibit line referencing mhd and wave_speeds).
- Around line 186-193: PHYSICS_DOCS is missing documentation for the new
wave_speeds constraint: update the "check_mhd" explanation to mention the new
requirement that wave_speeds = 1 for MHD and add a new "check_mhd_simulation"
entry in PHYSICS_DOCS describing riemann_solver = 1 (HLL) or 4 (HLLD),
wave_speeds must be 1 (Roe averages) and that wave_speeds = 2 is incompatible
with hyperbolic GLM cleaning, plus notes that HLLD is unavailable for RMHD and
hyperbolic cleaning requires 2D/3D; use the PHYSICS_DOCS dict and reference the
validator methods check_mhd and check_mhd_simulation so the auto-generated docs
reflect the new constraint.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/F057F8E6/golden-metadata.txt`:
- Line 13: The golden metadata shows "CMake v3.22.1 on LAPTOP-OMEN" (WSL2) which
may differ from CI and cause FP mismatches; re-run the test suite in the CI
environment (or reproduce the CI container/toolchain locally) to confirm the
committed goldens match CI results, and if they do not, regenerate the goldens
using the CI toolchain and update the golden files (or add CI-approved golden
artifacts) so that tests pass in CI; ensure the change references the metadata
entry "CMake v3.22.1 on LAPTOP-OMEN" when validating/regenerating.
- Line 7: The golden metadata shows the working tree was "(dirty)" for commit
b90fc2c922c770d767018bf7c7bf323c5dbe9e97 on branch fix/mhd-cleaning-speed;
inspect that commit and the working tree state by running a clean checkout of
that SHA (or checkout the branch and stash/reset local changes), then run git
status and git diff to identify any uncommitted changes; confirm any differences
are only environment/path items (e.g., local Fypp path or compiler wrapper) and
not code that affects physics/algorithms, regenerate the golden outputs from the
clean checkout and compare them to the current goldens (and update
tests/golden-metadata.txt only after proving reproducibility from the committed
tree) to ensure no unintended code changes were baked into the goldens.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1235 +/- ##
=======================================
Coverage 44.94% 44.95%
=======================================
Files 70 70
Lines 20504 20504
Branches 1946 1946
=======================================
+ Hits 9216 9217 +1
Misses 10166 10166
+ Partials 1122 1121 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codeant-ai
bot
added
size:L
Claude Code ReviewHead SHA: e553bbc Files changed: 8
Summary
Findings1. Validator check may be under-constrained — self.prohibit(mhd and wave_speeds is not None and wave_speeds == 2,
"MHD requires wave_speeds = 1")The error message says "MHD requires wave_speeds = 1", implying only value 1 is valid. But the guard only prohibits value 2. If other non-1 values (e.g., 3) exist or are reachable, they would silently pass. Consider: self.prohibit(mhd and wave_speeds is not None and wave_speeds != 1,
"MHD requires wave_speeds = 1")Minor consistency issue; not a correctness problem today if only values 1 and 2 are defined. 2. New hyper_cleaning block is inside the wave_speeds == 1 branch — The new 3. Golden metadata diffs are noisy — Metadata records machine-specific details (hostname, CPU, local paths). The actual OverallThe core physics fix is correct and well-motivated. Moving the cleaning speed constraint from |
Review Summary by QodoFix MHD hyperbolic cleaning wave speed sign and enforce wave_speeds validation
WalkthroughsDescription• Fixed MHD hyperbolic cleaning wave speed sign error in Riemann solver - Moved hyper-cleaning speed constraints from fast magnetosonic calculation to wave speed bounds - Changed left-state from min() to max() to ensure correct positive wave speeds • Added validator to enforce wave_speeds = 1 for MHD simulations - Prevents incompatible wave_speeds = 2 configuration with clear error message • Regenerated golden test files for MHD hyper-cleaning and rotor tests - Updated test metadata and outputs to reflect corrected cleaning behavior Diagramflowchart LR
A["MHD Riemann Solver"] -->|"Remove incorrect min/max<br/>from fast magnetosonic"| B["Fast Magnetosonic Speed"]
A -->|"Add hyper-cleaning constraints<br/>to wave speed bounds"| C["Wave Speed Bounds"]
C -->|"s_L = min(s_L, -c_h)<br/>s_R = max(s_R, c_h)"| D["Corrected Dedner GLM Eigenvalues"]
E["Configuration Validator"] -->|"Reject wave_speeds = 2<br/>for MHD"| F["Valid MHD Configuration"]
D -->|"Regenerate golden files"| G["Updated Test Outputs"]
File Changes1. src/simulation/m_riemann_solvers.fpp
|
Code Review by Qodo
1. Negative c_h flips bounds
|
Claude Code ReviewHead SHA: 8b37c79 Files changed: 8
Summary
Findings1. Incomplete validator condition —
|
Claude Code ReviewHead SHA: Files changed: 8
Summary
Findings[Medium — Correctness] The old code incorrectly applied [Low — Validator gap] The guard This would also catch any future unsupported values (3, 4, …) rather than only blocking 2. [Low — Golden metadata churn] The metadata files contain machine-specific paths (e.g., OverallThe core physics fix is correct and well-commented. The validator addition is a good guardrail. The minor validator gap (allowlist vs denylist) is worth tightening but is not blocking given that |
There was a problem hiding this comment.
Pull request overview
This PR improves stability and configuration safety for MHD runs with hyperbolic (Dedner/GLM) divergence cleaning by ensuring the Riemann fan bounds account for the cleaning wave speed and by rejecting incompatible wave_speeds settings for MHD cases.
Changes:
- Clamp computed Riemann wave-speed bounds (
s_L/s_R) to include the GLM cleaning eigenvalues±c_hwhenhyper_cleaningis enabled. - Add a case-validator constraint requiring
wave_speeds = 1for MHD simulations (rejectswave_speeds = 2). - Regenerate golden test metadata records for affected test cases.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/simulation/m_riemann_solvers.fpp |
Adjusts wave-speed bounding to include hyperbolic cleaning waves via s_L/s_R clamping. |
toolchain/mfc/case_validator.py |
Enforces an MHD-specific restriction on wave_speeds configuration. |
tests/F057F8E6/golden-metadata.txt |
Regenerated test metadata to reflect updated configuration/run. |
tests/EC8F87D9/golden-metadata.txt |
Regenerated test metadata to reflect updated configuration/run. |
tests/B4DC99F9/golden-metadata.txt |
Regenerated test metadata to reflect updated configuration/run. |
|
|
||
| self.prohibit(mhd and riemann_solver is not None and riemann_solver not in [1, 4], | ||
| "MHD simulations require riemann_solver = 1 (HLL) or riemann_solver = 4 (HLLD)") | ||
| self.prohibit(mhd and wave_speeds is not None and wave_speeds == 2, |
There was a problem hiding this comment.
The MHD wave_speeds constraint is currently written as wave_speeds == 2, which implicitly relies on the global wave_speeds domain staying limited to {1,2}. For clarity and future-proofing (and to match the existing model_eqns=3 check style), consider expressing this as wave_speeds is not None and wave_speeds != 1 so any non-1 value is rejected for MHD.
| self.prohibit(mhd and wave_speeds is not None and wave_speeds == 2, | |
| self.prohibit(mhd and wave_speeds is not None and wave_speeds != 1, |
| s_L = min(s_L, -hyper_cleaning_speed) | ||
| s_R = max(s_R, hyper_cleaning_speed) |
There was a problem hiding this comment.
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.
| 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)) |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| 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) |
There was a problem hiding this comment.
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
📝 WalkthroughWalkthroughThis pull request modifies MHD hyper_cleaning handling and adds validation enforcement. The Riemann solver changes relocate hyper_cleaning constraints from the fast magnetosonic speed computation to the wave-speed computation path, constraining left and right speeds when hyper_cleaning is enabled. A new validation rule is added to the case validator requiring wave_speeds equal to 1 when MHD is enabled. Additionally, multiple test metadata files are updated to reflect changes in build environment, compiler toolchain paths, Git branch references, and system architecture details from the test execution context. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ba926bc-29d7-4305-9c11-8389468b83b5
📒 Files selected for processing (8)
src/simulation/m_riemann_solvers.fpptests/B4DC99F9/golden-metadata.txttests/B4DC99F9/golden.txttests/EC8F87D9/golden-metadata.txttests/EC8F87D9/golden.txttests/F057F8E6/golden-metadata.txttests/F057F8E6/golden.txttoolchain/mfc/case_validator.py
| 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) |
There was a problem hiding this comment.
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.
User description
Placing my changes here as I don't have write access to #1180
CodeAnt-AI Description
Fix MHD hyperbolic cleaning wave sign and enforce correct MHD wave_speeds
What Changed
Impact
✅ Fewer MHD hyper-cleaning instabilities during runs✅ Clearer validation errors for incorrect MHD wave_speeds✅ Test outputs match corrected MHD cleaning behavior💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.