Fix MUSCL THINC right-state using already-overwritten left-state values#1181
Fix MUSCL THINC right-state using already-overwritten left-state values#1181sbryngelson wants to merge 2 commits intoMFlowCode:masterfrom
Conversation
|
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 · |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModifies MUSCL reconstruction in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
codeant-ai
bot
added
the
size:S
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Pull request overview
Fixes MUSCL-THINC right-state reconstruction by preserving pre-overwrite density ratios from the left state and reusing them for both left and right reconstructions.
Changes:
- Save pre-reconstruction density ratios (
rho_b,rho_e) before THINC overwritesvL_rs_vf_*values. - Update left/right reconstructions to use the saved ratios instead of dividing by potentially overwritten left-state fields.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
sbryngelson
force-pushed
the
fix/muscl-thinc-overwrite
branch
from
afa042e to
66a6a8c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1181 +/- ##
=======================================
Coverage 44.05% 44.06%
=======================================
Files 70 70
Lines 20496 20494 -2
Branches 1989 1989
=======================================
Hits 9030 9030
+ Misses 10328 10326 -2
Partials 1138 1138 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sbryngelson
requested review from
anandrdbz and
wilfonba
and removed request for
anandrdbz
|
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:S
|
CodeAnt AI Incremental review completed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/simulation/m_muscl.fpp`:
- Around line 250-256: The GPU parallel loop's private clause for the MUSCL
reconstruction is missing scalars A, B, and C which leads to cross-thread data
races; update the private list in the GPU_PARALLEL_LOOP invocation (the clause
currently listing j,k,l,aCL,aC,aCR,aTHINC,moncon,sign,qmin,qmax,rho_b,rho_e) to
also include A, B, and C so that the loop-local temporaries A, B, C used in the
MUSCL/THINC computation are private per thread (look for the muscl_dir
conditional and the GPU_PARALLEL_LOOP macro surrounding the loop body where
A/B/C are assigned and used).
- Around line 282-284: Guard against near-zero denominators when computing rho_b
and rho_e by clamping or early-checking the MUSCL-reconstructed state
vL_rs_vf_${XYZ}$ (j,k,l,advxb) using the existing sgm_eps (or ic_eps) tolerance:
ensure advxb is not below sgm_eps before dividing for rho_b and ensure (1 -
advxb) is not below sgm_eps before dividing for rho_e; if either check fails,
use a safe fallback (e.g., set rho_b/rho_e to a clipped value or the
cell-average aC) so that rho_b, rho_e computations in this block cannot produce
NaN/Inf.
| ! Save original density ratios before THINC overwrites them | ||
| rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/vL_rs_vf_${XYZ}$ (j, k, l, advxb) | ||
| rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb)) |
There was a problem hiding this comment.
Add defensive guards against near-zero denominators in rho_b / rho_e
The outer check guards the cell-average aC, but vL_rs_vf(j,k,l,advxb) is the MUSCL-reconstructed left state, which a slope limiter can legally push to values below ic_eps (for rho_b) or above 1 - ic_eps (making 1 - advxb ≈ 0 for rho_e). sgm_eps is already used for the same purpose on Line 278; applying it here is consistent and avoids NaN/Inf propagation in degenerate cells.
🛡️ Proposed fix
- rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/vL_rs_vf_${XYZ}$ (j, k, l, advxb)
- rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb))
+ rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/ &
+ max(vL_rs_vf_${XYZ}$ (j, k, l, advxb), sgm_eps)
+ rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/ &
+ max(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb), sgm_eps)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/simulation/m_muscl.fpp` around lines 282 - 284, Guard against near-zero
denominators when computing rho_b and rho_e by clamping or early-checking the
MUSCL-reconstructed state vL_rs_vf_${XYZ}$ (j,k,l,advxb) using the existing
sgm_eps (or ic_eps) tolerance: ensure advxb is not below sgm_eps before dividing
for rho_b and ensure (1 - advxb) is not below sgm_eps before dividing for rho_e;
if either check fails, use a safe fallback (e.g., set rho_b/rho_e to a clipped
value or the cell-average aC) so that rho_b, rho_e computations in this block
cannot produce NaN/Inf.
|
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:S
The right reconstruction divides by left-state values that were already overwritten during left reconstruction. Save original density ratios (contxb/advxb and contxe/(1-advxb)) before left reconstruction and reuse them for both left and right states. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
These scalars are written per-thread inside the parallel loop (lines 278-280) but were missing from the private list, causing a GPU data race on all backends. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sbryngelson
force-pushed
the
fix/muscl-thinc-overwrite
branch
from
9992699 to
ac18720
Compare
| ! Save original density ratios before THINC overwrites them | ||
| rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/vL_rs_vf_${XYZ}$ (j, k, l, advxb) | ||
| rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb)) |
There was a problem hiding this comment.
Suggestion: Guard the density ratio calculations against division by very small or zero volume fractions to avoid numerical instability in the interface-compression step. [custom_rule]
Severity Level: Minor
| ! Save original density ratios before THINC overwrites them | |
| rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/vL_rs_vf_${XYZ}$ (j, k, l, advxb) | |
| rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb)) | |
| ! Save original density ratios before THINC overwrites them, guarding against extreme volume fractions | |
| rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/max(vL_rs_vf_${XYZ}$ (j, k, l, advxb), ic_eps) | |
| rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/max(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb), ic_eps) |
Why it matters? ⭐
The change directly addresses a numerical correctness concern in the PR: the existing code divides by vL_rs_vf_${XYZ}$(..., advxb) and (1 - advxb) without guarding against those denominators being zero or extremely small. This can produce Inf/NaN on host or device, and this code runs inside a GPU-parallel loop (so device behavior matters). Replacing the denominator with max(..., ic_eps) prevents division-by-zero and reduces numerical instability while using the existing ic_eps constant already used elsewhere in the file. This maps to the repository review priorities (correctness/numerical issues and GPU safety) and is therefore an appropriate, minimal fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_muscl.fpp
**Line:** 282:284
**Comment:**
*Custom Rule: Guard the density ratio calculations against division by very small or zero volume fractions to avoid numerical instability in the interface-compression step.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
Claude Code ReviewHead SHA: 9992699 Files Changed (1)
Summary of Changes
FindingsNo bugs or CLAUDE.md violations found. The refactor is mathematically correct and equivalent to the original under normal conditions, while being more robust under THINC edge cases (aTHINC clamped to 0 or 1). Improvement Opportunities:
|
Claude Code ReviewHead SHA: ac18720 Files Changed (1)
Summary
FindingsNo bugs or CLAUDE.md violations found. The fix is mathematically correct and the GPU private clause is properly updated. Improvement opportunities:
Reviewed with Claude Code |
1 participant
User description
Summary
Severity: HIGH — right-state reconstruction uses overwritten left-state values.
File:
src/simulation/m_muscl.fpp, lines 285-301In the MUSCL-THINC interface reconstruction, the left-state values (
vL_rs_vfforcontxb,contxe, andadvxb) are overwritten during left reconstruction. The right-state reconstruction then divides by these already-overwritten values instead of the original cell-center values. While the math accidentally cancels (left THINC value / left THINC value = original ratio), this relies on exact cancellation and is fragile.Before
After
Why this went undetected
The algebraic cancellation makes this produce correct results in most cases. It would only fail if the THINC clamping pushed
aTHINCto exactly 0 or 1, causing 0/0.Test plan
🤖 Generated with Claude Code
Fixes #1202
Summary by CodeRabbit
CodeAnt-AI Description
Preserve original density ratios when computing MUSCL-THINC interface states
What Changed
Impact
✅ Avoids 0/0 errors during interface reconstruction✅ More reliable GPU runs (prevents data races)✅ Consistent left/right reconstructed interface states💡 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.