Conversation
…ry IBs, and removal of inner point array
Claude Code ReviewHead SHA: Summary
Findings1. Correctness — removed inner-points momentum-correction loop (high concern) Please confirm: is this loop genuinely redundant (e.g., because inner points are never touched by the Riemann solver), or does removing it change physics results? The testing note says "reran all mibm examples to ensure we did not run out of memory" — it would be valuable to also confirm that pressure/velocity fields inside IBs are unchanged. 2. Dead code — The only code that used 3. Exact allocation without buffer for non-moving case (low-risk but worth noting) For Minor
|
Claude Code ReviewHead SHA: 4150b12 Files changed: 1
Summary
Findings1. Medium —
|
Claude Code ReviewHead SHA: Files changed: 1
Summary
Findings1. Medium —
|
Claude Code ReviewHead SHA: Files changed: 1
Summary
Findings1. Unused variable
|
…nto ib-memory-reduction
Claude Code ReviewHead SHA: Summary
Findings[HIGH — Physics behavior change] Inner-point momentum zeroing removed without explicit justification The removed block: if (num_inner_gps > 0) then
do q = momxb, momxe
q_cons_vf(q)%sf(j, k, l) = 0._wp
end do
end ifzeroed momentum for interior IB points (cells fully inside an immersed boundary). These points still exist in the domain; they're just no longer tracked separately. Without the zeroing step, these cells will carry whatever conservative state the solver computes, which may be non-physical. The PR description says the inner-point array is "a waste of memory" but doesn't explain how the physics is preserved after removal. Please clarify: are interior-IB cells' states now handled elsewhere, or is zeroing them no longer necessary for correctness? [MEDIUM — Behavioral change] Old code only set [MEDIUM — Allocation sizing] Moving IBM buffer may be undersized in some MPI configurations call s_mpi_allreduce_integer_sum(num_gps, max_num_gps) ! sum across all ranks
max_num_gps = min(max_num_gps*2, (m + 1)*(n + 1)*(p + 1))
@:ALLOCATE(ghost_points(1:max_num_gps))
[LOW — Exact allocation for static case] For static IBM, Checklist items notedThe PR template GPU checklist boxes ( Overall: the memory reduction intent is sound and the implementation is clean. The primary concern is the silent physics change from dropping the inner-point momentum zeroing — please either explain why it's safe or restore it. The allocation sizing for multi-rank moving IBM also warrants a closer look. |
📝 WalkthroughWalkthroughThis change simplifies the IBM (Immersed Boundary Method) module by removing support for tracking inner points and inner GPU-related declarations. The implementation now relies exclusively on ghost points for tracking boundary interactions and GPU data management. Allocations, GPU data transfers, and data processing paths have been consolidated to use ghost points only. Two subroutine signatures were updated: 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
| $:GPU_ENTER_DATA(copyin='[num_gps]') | ||
|
|
There was a problem hiding this comment.
Missing GPU_EXIT_DATA for num_gps.
num_gps is entered to GPU data region here but there's no corresponding GPU_EXIT_DATA in s_finalize_ibm_module to release the device memory.
| @:ALLOCATE(ghost_points(1:max_num_gps)) | ||
|
|
||
| $:GPU_ENTER_DATA(copyin='[ghost_points,inner_points]') | ||
| call s_find_ghost_points(ghost_points, inner_points) | ||
| $:GPU_ENTER_DATA(copyin='[ghost_points]') | ||
| call s_find_ghost_points(ghost_points) |
There was a problem hiding this comment.
Missing @:DEALLOCATE and GPU_EXIT_DATA for ghost_points.
ghost_points is allocated here with @:ALLOCATE and entered to GPU with GPU_ENTER_DATA, but s_finalize_ibm_module does not include matching cleanup calls. This will cause a memory leak on both host and device.
🔧 Proposed fix in s_finalize_ibm_module
impure subroutine s_finalize_ibm_module()
+ $:GPU_EXIT_DATA(delete='[ghost_points, num_gps]')
+ @:DEALLOCATE(ghost_points)
@:DEALLOCATE(ib_markers%sf)
if (allocated(airfoil_grid_u)) then
@:DEALLOCATE(airfoil_grid_u)
@:DEALLOCATE(airfoil_grid_l)
end if
end subroutine s_finalize_ibm_moduleAs per coding guidelines: "Every @:ALLOCATE(...) macro call must have a matching @:DEALLOCATE(...) in Fortran files" and "Lifecycle of allocations must be paired with deallocations; ensure all data entering GPU memory has corresponding exit/cleanup."
Claude Code ReviewHead SHA: 58ac6bc Summary
Findings1. Encoded
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1307 +/- ##
==========================================
- Coverage 45.36% 45.36% -0.01%
==========================================
Files 70 70
Lines 20499 20488 -11
Branches 1953 1951 -2
==========================================
- Hits 9300 9294 -6
+ Misses 10074 10069 -5
Partials 1125 1125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Before scaling up, I was looking for any free memeory that we have been loose with and cleaning it up.
This removes the inner point array, as it is just a waste of memory resources. It also means we can reduce the number of ghost points to keep in memory. I also added protection preventing arbitrary memory allocation in non-moving cases.
Type of change
Testing
How did you test your changes?
I reran all mibm examples to ensure we did not run out of memory
GPU changes (expand if you modified
src/simulation/)AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR:
@coderabbitai review— incremental review (new changes only)@coderabbitai full review— full review from scratch/review— Qodo review/improve— Qodo code suggestions