Fix hyper_cleaning psi initialization only covering k=0 plane#1217
Fix hyper_cleaning psi initialization only covering k=0 plane#1217sbryngelson wants to merge 3 commits intoMFlowCode:masterfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 · |
📝 WalkthroughWalkthroughThe hyper_cleaning initialization for the psi field is expanded from a 2D loop covering only the z=0 plane to a 3D loop iterating over all z-planes. The Gaussian expression now incorporates the z coordinate when p > 0, while maintaining backward compatibility for 2D simulations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Pull request overview
Fixes MHD hyperbolic divergence cleaning (hyper_cleaning) psi initialization so 3D simulations initialize all z-planes (not just z=0), making the Gaussian initialization spatially consistent in 3D.
Changes:
- Add a z-loop (
l = 0, p) for psi initialization. - Include
z_cc(l)in the Gaussian exponent and initializesf(j,k,l)for all planes.
z_cc is only allocated when p > 0. The previous commit accessed z_cc(l) unconditionally, which would crash or read garbage in 2D runs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace `1d-2` (hard double precision) and bare `2.0`/`0.05` (default real) with `_wp`-suffixed literals so the Gaussian initialization is consistent with the working precision across single and double builds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pre_process/m_start_up.fpp (1)
789-801: Hoist the loop-invariantif (p > 0)branch outside all three loops.
pis a global constant — the branch in the innermost loop is re-evaluated(m+1)×(n+1)×(p+1)times unnecessarily, and the intent is clearer when the 3D vs. 2D paths are separated at the outermost level.♻️ Proposed refactor
- do l = 0, p - do k = 0, n - do j = 0, m - if (p > 0) then - q_cons_vf(psi_idx)%sf(j, k, l) = 1.0e-2_wp*exp(-(x_cc(j)**2 + y_cc(k)**2 + z_cc(l)**2)/(2.0_wp*0.05_wp**2)) - else - q_cons_vf(psi_idx)%sf(j, k, l) = 1.0e-2_wp*exp(-(x_cc(j)**2 + y_cc(k)**2)/(2.0_wp*0.05_wp**2)) - end if - q_prim_vf(psi_idx)%sf(j, k, l) = q_cons_vf(psi_idx)%sf(j, k, l) - end do - end do - end do + if (p > 0) then + do l = 0, p + do k = 0, n + do j = 0, m + q_cons_vf(psi_idx)%sf(j, k, l) = 1.0e-2_wp*exp(-(x_cc(j)**2 + y_cc(k)**2 + z_cc(l)**2)/(2.0_wp*0.05_wp**2)) + q_prim_vf(psi_idx)%sf(j, k, l) = q_cons_vf(psi_idx)%sf(j, k, l) + end do + end do + end do + else + do k = 0, n + do j = 0, m + q_cons_vf(psi_idx)%sf(j, k, 0) = 1.0e-2_wp*exp(-(x_cc(j)**2 + y_cc(k)**2)/(2.0_wp*0.05_wp**2)) + q_prim_vf(psi_idx)%sf(j, k, 0) = q_cons_vf(psi_idx)%sf(j, k, 0) + end do + end do + end if🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pre_process/m_start_up.fpp` around lines 789 - 801, The innermost branch testing p (>0) should be hoisted outside the triple loop to avoid re-evaluating a loop-invariant; when hyper_cleaning is true, split into two separate nested loops: one for the 3D case that uses z_cc(l) and sets q_cons_vf(psi_idx)%sf and q_prim_vf(psi_idx)%sf with exp(-(x_cc(j)**2 + y_cc(k)**2 + z_cc(l)**2)/(...)), and one for the 2D case that omits z_cc(l) and uses exp(-(x_cc(j)**2 + y_cc(k)**2)/(...)); keep the existing hyper_cleaning guard and the assignments to q_cons_vf and q_prim_vf and reference psi_idx, x_cc, y_cc, z_cc, m, n, p to locate and restructure the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pre_process/m_start_up.fpp`:
- Around line 789-801: The innermost branch testing p (>0) should be hoisted
outside the triple loop to avoid re-evaluating a loop-invariant; when
hyper_cleaning is true, split into two separate nested loops: one for the 3D
case that uses z_cc(l) and sets q_cons_vf(psi_idx)%sf and q_prim_vf(psi_idx)%sf
with exp(-(x_cc(j)**2 + y_cc(k)**2 + z_cc(l)**2)/(...)), and one for the 2D case
that omits z_cc(l) and uses exp(-(x_cc(j)**2 + y_cc(k)**2)/(...)); keep the
existing hyper_cleaning guard and the assignments to q_cons_vf and q_prim_vf and
reference psi_idx, x_cc, y_cc, z_cc, m, n, p to locate and restructure the code.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1217 +/- ##
==========================================
- Coverage 44.05% 44.05% -0.01%
==========================================
Files 70 70
Lines 20498 20501 +3
Branches 1990 1991 +1
==========================================
+ Hits 9030 9031 +1
- Misses 10329 10330 +1
- Partials 1139 1140 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sbryngelson Thanks for fixing my oversight. The new 3D initialization looks good |
User description
Summary
hyper_cleaning) psi field initialization inm_start_up.fppto cover all z-planes in 3D simulations, not just thez=0plane.Bug Details
The psi initialization loop hardcodes the third array index to
0:In 3D simulations (
p > 0), all z-planes beyondz=0are left uninitialized (containing whatever value they had from prior initialization or garbage). This corrupts the divergence cleaning field in 3D MHD simulations, as the psi field would be zero (or uninitialized) everywhere except the first z-plane.Fix
Add an outer loop over the z-dimension (
l = 0, p) and includez_cc(l)in the Gaussian exponent so the 3D initialization is physically consistent. For 2D cases (p=0), the loop executes once withl=0, preserving existing behavior.Test plan
🤖 Generated with Claude Code
CodeAnt-AI Description
Initialize hyperbolic cleaning psi across all z-planes in 3D
What Changed
Impact
✅ Fewer uninitialized psi values in 3D initial conditions✅ Fewer 3D divergence-cleaning artifacts during startup✅ Preserves existing 2D initialization 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.
Fixes #1221
Summary by CodeRabbit