Open
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>
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughMoves the hyper_cleaning wave-speed clamp into the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
2 tasks
codeant-ai
bot
added
the
size:L
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
Nitpicks 🔍
|
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.
Contributor
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.05% 44.06%
=======================================
Files 70 70
Lines 20496 20496
Branches 1989 1989
=======================================
+ Hits 9030 9031 +1
Misses 10328 10328
+ Partials 1138 1137 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:L
Contributor
|
CodeAnt AI Incremental review completed. |
Claude Code ReviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Claude Code ReviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Placing my changes here as I don't have write access to #1180
Summary by CodeRabbit
Improvements
New Features
Tests
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.