Skip to content

Comments

Fix MHD hyperbolic cleaning using wrong sign for left fast magnetosonic speed#1180

Closed
sbryngelson wants to merge 2 commits intoMFlowCode:masterfrom
sbryngelson:fix/mhd-cleaning-speed
Closed

Fix MHD hyperbolic cleaning using wrong sign for left fast magnetosonic speed#1180
sbryngelson wants to merge 2 commits intoMFlowCode:masterfrom
sbryngelson:fix/mhd-cleaning-speed

Conversation

@sbryngelson
Copy link
Member

@sbryngelson sbryngelson commented Feb 21, 2026

User description

Summary

Severity: HIGH — wrong wave speed estimate corrupts MHD flux computation.

File: src/simulation/m_riemann_solvers.fpp, lines 655-657

The hyperbolic divergence cleaning should ensure that the fast magnetosonic speed is at least the cleaning speed in both directions. The right state correctly uses max(c_fast%R, hyper_cleaning_speed), but the left state uses min(c_fast%L, -hyper_cleaning_speed), which forces it negative.

Before

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

After

if (hyper_cleaning) then
    c_fast%L = max(c_fast%L, hyper_cleaning_speed)    ! symmetric with R
    c_fast%R = max(c_fast%R, hyper_cleaning_speed)
end if

Why this went undetected

MHD with hyperbolic cleaning is a specialized feature. The asymmetric wave speed produces subtly wrong fluxes that may not cause immediate crashes.

Test plan

  • Run MHD simulation with hyperbolic cleaning enabled
  • Verify wave speed estimates are symmetric between left and right states

🤖 Generated with Claude Code

Fixes #1201


CodeAnt-AI Description

Fix wrong sign for left fast magnetosonic speed in MHD hyperbolic cleaning

What Changed

  • When hyperbolic divergence cleaning is enabled, the left-state fast magnetosonic speed is now forced to be at least the cleaning speed (previous code could force it negative); this matches the right-state treatment and prevents asymmetric wave-speed estimates.
  • Regenerated golden test outputs for MHD hyper-cleaning and rotor tests so CI reflects the corrected wave-speed behavior.

Impact

✅ Correct MHD fluxes when hyperbolic cleaning is enabled
✅ Fewer corrupted or asymmetric wave solutions in affected simulations
✅ Updated regression goldens to match corrected 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:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

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:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

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.

Summary by CodeRabbit

  • Bug Fixes

    • Refined wave-speed computation in MHD simulations for improved numerical stability.
  • Tests

    • Updated test infrastructure and build configurations.

Copilot AI review requested due to automatic review settings February 21, 2026 03:23
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 21, 2026

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 ·
Reddit ·
LinkedIn

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 21, 2026

📝 Walkthrough

Walkthrough

This PR fixes a sign error in the MHD Riemann solver's hyperbolic divergence cleaning wave-speed bound calculation. The left fast magnetosonic speed bound changes from a minimum operation with negated speed to a maximum operation with positive speed. Test metadata files are updated to reflect a new build environment and hardware platform.

Changes

Cohort / File(s) Summary
MHD Riemann Solver Logic
src/simulation/m_riemann_solvers.fpp
Fixed sign error in left fast magnetosonic speed hyper-cleaning bound: changed from min(c_fast%L, -hyper_cleaning_speed) to max(c_fast%L, hyper_cleaning_speed) to correctly enforce the positive speed bound.
Test Metadata
tests/B4DC99F9/golden-metadata.txt, tests/F057F8E6/golden-metadata.txt
Updated test metadata files with new CMake version (v3.26.5), GCC toolchain (v12.3.0), hardware platform details (Intel Xeon Gold 6226), and build configuration toggles (MPI OFF, SIMULATION ON). Fypp path and section ordering also updated to reflect new build environment.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • MFlowCode/MFC#1054: Modifies Riemann solver wave-speed and flux logic in the same file with broader indexing/flux refactoring alongside this hyper-cleaning bound fix.

Suggested labels

bug-fix, physics, MHD

Poem

🐰 A sign flip heals the cleaning flow,
Where left and right now truly know,
Max bounds the speeds with proper care,
The MHD solver's back in repair! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive While the core fix in m_riemann_solvers.fpp aligns with issue #1201, the extensive regeneration of golden metadata files for two unrelated test cases appears tangential to the primary bug fix. Clarify whether the golden file regenerations were necessary to validate the corrected wave-speed behavior, or if they should be addressed in a separate test-update PR.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and specifically identifies the bug fix: correcting the wrong sign used for the left fast magnetosonic speed in MHD hyperbolic cleaning.
Linked Issues check ✅ Passed The PR successfully addresses issue #1201 by changing the left-state clamping from min(c_fast%L, -hyper_cleaning_speed) to max(c_fast%L, hyper_cleaning_speed), meeting the stated objective to fix the MHD hyperbolic cleaning wave speed calculation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The pull request description comprehensively documents the bug, provides clear before/after code examples, explains the severity and impact, and includes a test plan.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codeant-ai codeant-ai bot added the size:XS This PR changes 0-9 lines, ignoring generated files label Feb 21, 2026
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 21, 2026

CodeAnt AI finished reviewing your PR.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes the sign handling for left-state fast magnetosonic speed enforcement when hyperbolic cleaning is enabled, making it consistent with the right-state treatment.

Changes:

  • Replace min(c_fast%L, -hyper_cleaning_speed) with max(c_fast%L, hyper_cleaning_speed) under hyperbolic cleaning.
  • Keep right-state enforcement as max(c_fast%R, hyper_cleaning_speed) for symmetry.

cubic-dev-ai[bot]

This comment was marked as off-topic.

…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
Copy link
Contributor

codeant-ai bot commented Feb 22, 2026

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 ·
Reddit ·
LinkedIn

@codeant-ai codeant-ai bot added size:L This PR changes 100-499 lines, ignoring generated files and removed size:XS This PR changes 0-9 lines, ignoring generated files labels Feb 22, 2026
@codeant-ai
Copy link
Contributor

codeant-ai bot commented Feb 22, 2026

CodeAnt AI Incremental review completed.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 6: The CI is running test F057F8E6 with MPI=ON by default while the
golden metadata shows mpi=No; update the CI matrix entry for the test id
F057F8E6 to force mpi=no-mpi (set the matrix key "mpi" to the no-mpi variant for
that job), or alternatively add F057F8E6 to the test exclusions when mpi is ON,
or regenerate the golden for both mpi=Yes and mpi=No so CI and the golden
configs match. Ensure you modify the workflow matrix "mpi" setting or the test
include/exclude entries that reference F057F8E6 accordingly.

---

Duplicate comments:
In `@tests/B4DC99F9/golden-metadata.txt`:
- Line 7: The golden metadata for the mhd_rotor test records a dirty git state
("(dirty)"), indicating uncommitted changes at generation; reproduce a
clean-state golden by checking out a clean commit/branch, committing or stashing
any local changes, regenerating the golden output, and updating the metadata so
the Git line for mhd_rotor no longer contains "(dirty)"; reference the golden
identifier "mhd_rotor" and the related test metadata file and follow the same
verification/cleanup steps used for the previously reported F057F8E6 occurrence
to ensure parity.

@codecov
Copy link

codecov bot commented Feb 22, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 44.05%. Comparing base (84c46e0) to head (36bf826).

Files with missing lines Patch % Lines
src/simulation/m_riemann_solvers.fpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1180   +/-   ##
=======================================
  Coverage   44.05%   44.05%           
=======================================
  Files          70       70           
  Lines       20498    20498           
  Branches     1990     1990           
=======================================
  Hits         9030     9030           
  Misses      10329    10329           
  Partials     1139     1139           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ChrisZYJ
Copy link
Contributor

@sbryngelson Thanks for fixing this. Hyperbolic cleaning expands the HLL fan to include the new eigenvalues. In less extreme cases, the wrong formulas had little impact, so the code ran fine with my examples. I've added a few more changes, and now it matches theory and is fully robust.

I don't seem to have write access to this branch, so I'm opening another PR #1235.

@sbryngelson
Copy link
Member Author

Closing in favor of #1235, which incorporates this fix and adds additional MHD wave speed improvements.

@sbryngelson sbryngelson deleted the fix/mhd-cleaning-speed branch February 23, 2026 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files

Development

Successfully merging this pull request may close these issues.

MHD hyperbolic cleaning uses wrong sign for left fast magnetosonic speed

2 participants