Fix 6 low-risk pre-process bugs (batch)#1241
Fix 6 low-risk pre-process bugs (batch)#1241sbryngelson wants to merge 17 commits intoMFlowCode:masterfrom
Conversation
codeant-ai
bot
added
the
size:S
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Nitpicks 🔍
|
There was a problem hiding this comment.
Pull request overview
This PR batches 6 bug fixes in pre-process code paths that address real numerical issues in initial condition generation. The fixes correct type declarations, array bounds, duplicate operations, statement ordering, loop coverage, and MPI broadcast placement.
Changes:
- Fixed
moncon_cutoffdeclared asintegerinstead ofreal(wp), which silently truncated1e-8to0and disabled MUSCL-THINC monotonicity limiting - Corrected grid stretching array bounds for
y_ccandz_ccusing wrong dimension (minstead ofnandp) - Removed duplicate R3bar accumulation line that doubled bubble perturbation magnitude in QBMM initial conditions
- Reordered perturbation statements so y-velocity uses original x-velocity before x-velocity is modified
- Extended hyper-cleaning psi initialization to cover all z-planes in 3D (was only initializing k=0 plane)
- Moved IB patch velocity/angular_vel/angles broadcasts from wrong loop to correct loop
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/common/m_constants.fpp | Changed moncon_cutoff from integer to real(wp) to prevent truncation |
| src/pre_process/m_grid.f90 | Fixed y_cc and z_cc array bounds to use correct dimensions (n and p instead of m) |
| src/pre_process/m_assign_variables.fpp | Removed duplicate R3bar accumulation line in QBMM branch |
| src/pre_process/m_perturbation.fpp | Swapped statement order so y-velocity perturbation uses unmodified x-velocity |
| src/pre_process/m_start_up.fpp | Added outer z-loop for 3D psi initialization and replaced double-precision literal |
| src/pre_process/m_mpi_proxy.fpp | Moved patch_ib vel/angular_vel/angles broadcasts to num_patches_max loop |
codeant-ai
bot
added
size:S
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1241 +/- ##
==========================================
- Coverage 44.04% 44.03% -0.01%
==========================================
Files 70 70
Lines 20499 20503 +4
Branches 1993 1996 +3
==========================================
+ Hits 9028 9029 +1
- Misses 10330 10333 +3
Partials 1141 1141 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codeant-ai
bot
added
size:S
![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
🤖 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/pre_process/m_start_up.fpp`:
- Around line 790-797: Add a validator in the CaseValidator
(toolchain/mfc/case_validator.py) to forbid enabling hyper_cleaning in 2D: call
self.prohibit(hyper_cleaning and p is not None and p == 0, "Hyperbolic cleaning
is not supported for 2D simulations") in the validation routine that checks
dimensionality (the same place that already forbids 1D via n==0); this prevents
m_start_up.fpp from accessing unallocated z_cc when p==0 by rejecting
hyper_cleaning for 2D cases.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/common/m_constants.fppsrc/pre_process/m_assign_variables.fppsrc/pre_process/m_grid.f90src/pre_process/m_mpi_proxy.fppsrc/pre_process/m_perturbation.fppsrc/pre_process/m_start_up.fpp
💤 Files with no reviewable changes (1)
- src/pre_process/m_assign_variables.fpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/m_constants.fpp
- src/pre_process/m_mpi_proxy.fpp
sbryngelson
force-pushed
the
fix/low-risk-bugfixes-batch
branch
from
285998d to
068c79c
Compare
codeant-ai
bot
added
size:S
sbryngelson
force-pushed
the
fix/low-risk-bugfixes-batch
branch
2 times, most recently
from
9276e0b to
96f562c
Compare
codeant-ai
bot
added
size:S
sbryngelson
force-pushed
the
fix/low-risk-bugfixes-batch
branch
from
1dcf0a3 to
22304a1
Compare
moncon_cutoff is assigned 1e-8_wp but declared as integer, so Fortran silently truncates it to 0. This makes all THINC monotonicity constraint comparisons always true, completely disabling the constraint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
y_cc(0:m) and z_cc(0:m) use the x-dimension size m instead of the y-dimension n and z-dimension p respectively. Corrupts cell-center coordinates when grid stretching is enabled and m != n or m != p. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion Character-for-character identical duplicate line causes R3bar to always be 2x the intended value for QBMM cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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>
patch_ib vel, angular_vel, and angles were broadcast inside the num_bc_patches_max loop instead of the num_patches_max loop where the other patch_ib fields are broadcast. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove redundant if(p>0) branch in psi initialization: z_cc(0)=0 in 2D, so the 3D formula produces identical results in both cases. - Replace hardcoded 3 with size(patch_ib(i)%VAR) in MPI_BCAST for vel/angular_vel/angles, consistent with other IB broadcasts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Build the Gaussian r^2 incrementally, guarding y_cc(k) behind n > 0 and z_cc(l) behind p > 0, since these arrays are only allocated in 2D+ and 3D respectively. Prevents undefined behavior if hyper_cleaning is enabled in a 1D MHD configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
src/pre_process/m_start_up.fpp: - Add @:ASSERT(psi_idx > 0) before hyper_cleaning block to catch misconfigured cases where hyper_cleaning is enabled but psi_idx is unset src/pre_process/m_mpi_proxy.fpp: - Add comment explaining why IB patch broadcasts are in the num_patches_max loop rather than the num_bc_patches_max loop (patch_ib is sized by num_patches_max, not num_bc_patches_max) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The psi field used by hyperbolic cleaning is initialized with a 3D loop that accesses z_cc, which is out-of-bounds when p == 0. The 1D prohibition already existed; extend it to 2D. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In 1D simulations n==0 and p==0, so both the 1D and 2D prohibition checks were firing simultaneously. Gate the 2D check on n > 0 so it only triggers for configurations that are actually 2D. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GLM divergence cleaning is valid in 2D. The z_cc access was already guarded with `if (p > 0)` in 22304a1. The prohibition broke the existing 2D hyper_cleaning, mhd_rotor, and orszag_tang tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The @:ASSERT call added in d614938 requires the macros.fpp include, which was missing, causing Fypp preprocessing to fail in the documentation build. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sbryngelson
force-pushed
the
fix/low-risk-bugfixes-batch
branch
from
df91890 to
732146c
Compare
The linter will intentionally fail until PRs MFlowCode#1187 (WP_MOK), MFlowCode#1241 (R3bar duplicate), and this PR's dq_prim_dy fix (now in MFlowCode#1187) merge first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Summary by QodoFix six low-risk pre-process bugs in IC generation
WalkthroughsDescription• Fixes moncon_cutoff type from integer to real, restoring THINC monotonicity limiting • Corrects stretched-grid cell-center bounds for y and z dimensions using proper array sizes • Removes duplicate R3bar accumulation that doubled QBMM bubble perturbation magnitude • Fixes y-velocity perturbation order to use unmodified x-velocity values • Extends hyper-cleaning psi initialization across all 3D planes with proper 2D guards • Moves immersed-boundary variable broadcasts to correct loop scope for proper MPI propagation Diagramflowchart LR
A["Pre-process bugs"] --> B["moncon_cutoff type fix"]
A --> C["Grid stretching bounds"]
A --> D["QBMM perturbation"]
A --> E["Velocity perturbation order"]
A --> F["Hyper-cleaning 3D init"]
A --> G["IB broadcast scope"]
B --> H["THINC limiting restored"]
C --> I["Correct coordinates"]
D --> J["Correct amplitude"]
E --> K["Correct values"]
F --> L["Full 3D coverage"]
G --> M["Reliable MPI sync"]
File Changes1. src/common/m_constants.fpp
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewⓘ The new review experience is currently in Beta. Learn more |
Claude Code ReviewHead SHA: Summary
FindingsNo blocking issuesAll six fixes look correct. Detailed per-fix notes:
Minor / Non-blocking
VerdictAll fixes are correct, well-scoped, and adhere to MFC conventions. Approved. |
Claude Code ReviewHead SHA: Files changed: 6
Summary
Findings1.
|
|
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 (6)
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 |
User description
Summary
Batches 6 low-risk bug fixes in pre-process code paths. These fix real numerical issues in IC generation but don't touch simulation GPU kernels or MPI hot paths.
Included fixes
moncon_cutofftype (Fix moncon_cutoff declared as integer, truncating 1e-8 to 0 #1174): Declared asinteger, silently truncating1e-8to0— disabled MUSCL-THINC monotonicity limitery_cc(0:m)andz_cc(0:m)used x-dimension bound instead of0:n/0:ppsiinitialization only coveredk=0plane in 3D MHD casesvel/angular_vel/anglesbroadcast was inside the wrong loopSupersedes
Closes #1174, closes #1179, closes #1183, closes #1216, closes #1217, closes #1188, fixes #1197, fixes #1202, fixes #1206, fixes #1208, fixes #1221, fixes #1222
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Validation
Chores
CodeAnt-AI Description
Fix pre-process bugs that caused incorrect initial conditions and MPI broadcasts
What Changed
Impact
✅ Correct THINC interface limiting✅ Accurate stretched-grid coordinates✅ Correct QBMM bubble perturbation amplitudes💡 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.