Fix mv read loop hardcoding 4 instead of nnode#1190
Fix mv read loop hardcoding 4 instead of nnode#1190sbryngelson wants to merge 5 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 · |
📝 WalkthroughWalkthroughThe pull request systematically replaces hardcoded multiplier 4 with the dynamic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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:XS
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the bubble restart file reading logic where the mv data file loop was hardcoded to iterate 4 times instead of using the nnode variable, which could cause incorrect file reads when nnode != 4.
Changes:
- Updated the
mvdata file loop to usennodeinstead of the hardcoded value4 - Corrected the file number offset calculation to use
nnodefor proper indexing
![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.
The pb loop just above correctly uses nnode for both the loop bound and file number offset. The mv loop should match. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sbryngelson
force-pushed
the
fix/mv-loop-nnode
branch
from
44e0f7f to
0870de7
Compare
…ommon Fix all remaining instances of the literal integer 4 representing the QBMM quadrature node count (nnode) across simulation and common modules, completing the nnode parameterization started in the PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix remaining instances of the literal 4 representing QBMM quadrature node count in pre_process/m_global_parameters.fpp and m_start_up.fpp, consistent with the fixes already applied to common and simulation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1190 +/- ##
=======================================
Coverage 44.05% 44.05%
=======================================
Files 70 70
Lines 20496 20496
Branches 1989 1989
=======================================
Hits 9030 9030
Misses 10328 10328
Partials 1138 1138 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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:M
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (bubbles_euler .and. qbmm .and. .not. polytropic) then | ||
| do i = sys_size + 1, sys_size + 2*nb*4 | ||
| do i = sys_size + 1, sys_size + 2*nb*nnode | ||
| allocate (MPI_IO_DATA%var(i)%sf(0:m, 0:n, 0:p)) |
There was a problem hiding this comment.
Suggestion: In the loop initializing the extended MPI_IO_DATA%var range for the bubbles_euler .and. qbmm .and. .not. polytropic case, sf is first allocated and then immediately pointer-assigned to null(), which discards the only reference to the newly allocated memory and causes a memory leak for each i in that range; since sf is later used as a pointer to other targets, the allocation here is unnecessary and should be removed so only the nullification remains. [memory leak]
Severity Level: Major ⚠️
- ⚠️ Extra MPI_IO_DATA extended-range buffers leaked at initialization.
- ⚠️ Increased memory footprint for qbmm Euler bubble runs.| allocate (MPI_IO_DATA%var(i)%sf(0:m, 0:n, 0:p)) |
Steps of Reproduction ✅
1. Run any simulation configuration that sets `bubbles_euler = .true.`, `qbmm = .true.`,
and `polytropic = .false.` in the input, causing the non-polytropic Eulerian bubble + qbmm
path to be active when `m_global_parameters` is initialized.
2. During startup, the driver calls `s_initialize_global_parameters_module` in
`src/simulation/m_global_parameters.fpp` (see around lines 1100–1400), which computes
`sys_size` and then executes the allocation block at lines 1235–1238 for
`MPI_IO_DATA%var(sys_size+1:sys_size+2*nb*nnode)%sf`.
3. For each `i` in `sys_size + 1, ..., sys_size + 2*nb*nnode`, the statement `allocate
(MPI_IO_DATA%var(i)%sf(0:m, 0:n, 0:p))` allocates a new target and associates the pointer
component `sf` with it, and the next line `MPI_IO_DATA%var(i)%sf => null()` immediately
disassociates `sf` from that target.
4. Because no other pointer references the just-allocated targets and there is no
subsequent `deallocate` for them (finalization in `s_finalize_global_parameters_module`
only nullifies `sf` again; see around lines 1500–1560), those allocations become
unreachable and remain allocated until process termination, which can be confirmed by
running the executable under a memory debugger (e.g., Valgrind) and observing leaked
blocks originating from `m_global_parameters.fpp:1236`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_global_parameters.fpp
**Line:** 1236:1236
**Comment:**
*Memory Leak: In the loop initializing the extended `MPI_IO_DATA%var` range for the `bubbles_euler .and. qbmm .and. .not. polytropic` case, `sf` is first allocated and then immediately pointer-assigned to `null()`, which discards the only reference to the newly allocated memory and causes a memory leak for each `i` in that range; since `sf` is later used as a pointer to other targets, the allocation here is unnecessary and should be removed so only the nullification remains.
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 ReviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pre_process/m_start_up.fpp (1)
417-420:⚠️ Potential issue | 🟡 Minor
file_numbuffer may be too short forsys_size + nb*nnodewhennnodeis large.The buffer is sized for the digit count of
sys_sizealone, but numbers written during pb/mv reads go up tosys_size + nb*nnode. For the current default (nnode = 4) this is benign becausesys_sizealready incorporatesnb*nmom(which exceedsnb*4). However, for largernnodevalues — which this PR explicitly enables — the number of digits can increase, causing Fortran to fillfile_numwith'*'characters and silently failing to find the data file.🐛 Proposed fix
-character(LEN= & - int(floor(log10(real(sys_size, wp)))) + 1) :: file_num +character(LEN= & + int(floor(log10(real(sys_size + 2*nb*nnode, wp)))) + 1) :: file_num🤖 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 417 - 420, The declaration for file_num is too short because it only uses digits of sys_size; change the sizing to compute the maximum possible index (e.g. max_index = sys_size + nb*nnode) and use int(floor(log10(real(max_index, wp))) + 1) for the character(LEN=...) length so file_num can hold numbers up to sys_size + nb*nnode; update any related initializations that assume the old length and keep the symbol names file_num, sys_size, nb, nnode, and wp to locate and modify the declaration and any dependent code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pre_process/m_start_up.fpp`:
- Around line 417-420: The declaration for file_num is too short because it only
uses digits of sys_size; change the sizing to compute the maximum possible index
(e.g. max_index = sys_size + nb*nnode) and use int(floor(log10(real(max_index,
wp))) + 1) for the character(LEN=...) length so file_num can hold numbers up to
sys_size + nb*nnode; update any related initializations that assume the old
length and keep the symbol names file_num, sys_size, nb, nnode, and wp to locate
and modify the declaration and any dependent code.
1 participant
User description
Summary
Severity: HIGH — mv data file loop uses wrong bounds and offsets when nnode != 4.
File:
src/pre_process/m_start_up.fpp, lines 479-482The
pb(pressure) read loop just above correctly usesnnodefor both the loop bound and file number offset calculation. Themv(mass/velocity) read loop immediately below hardcodes4instead.Before
After
Why this went undetected
nnodeis 4 in the most common configuration, so the hardcoded value happens to be correct.Test plan
🤖 Generated with Claude Code
Fixes #1210
CodeAnt-AI Description
Fix reading and MPI I/O for QBMM variables when quadrature node count is not 4
What Changed
Impact
✅ Correct mv file reads with non-default nnode✅ Fewer MPI I/O size/view mismatches for QBMM variables✅ Correct buffer packing/unpacking for variable node counts💡 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.
Summary by CodeRabbit