Analytic mibm velocities and airfoil centroid #1111
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 · |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds per-patch Changes
Sequence Diagram(s)sequenceDiagram
participant TS as "TimeStepper"
participant Prop as "s_propagate_immersed_boundaries(s)"
participant Analytic as "Analytic (mib_analytical / Case)"
participant IBM as "s_compute_ib_forces / s_compute_centroid_offset"
participant Patch as "patch_ib[]"
participant MPI as "MPI Allreduce"
TS->>Prop: call s_propagate_immersed_boundaries(s)
Prop->>Analytic: if moving_ibm == 1 → evaluate analytic motion
Analytic->>Patch: write velocities/angles to patch_ib fields
Prop->>IBM: if two-way → call s_compute_ib_forces and s_compute_centroid_offset
IBM->>Patch: accumulate per-cell mass*pos and counts
IBM->>MPI: Allreduce mass*pos and counts
MPI-->>IBM: reduced totals
IBM->>Patch: compute center_of_mass, set patch_ib%centroid_offset (rotate to local frame)
Prop->>TS: update IB states for timestep
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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
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:L
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| do i = 0, m | ||
| xyz_local = [x_cc(i) - center(1), y_cc(j) - center(2), z_cc(l) - center(3)] ! get coordinate frame centered on IB | ||
| xyz_local = matmul(inverse_rotation, xyz_local) ! rotate the frame into the IB's coordinates | ||
| xyz_local = xyz_local - patch_ib(patch_id)%centroid_offset ! airfoils are a patch that require a centroid offset |
There was a problem hiding this comment.
Suggestion: The 3D airfoil routine subtracts patch_ib(patch_id)%centroid_offset from xyz_local after applying inverse_rotation; if centroid_offset is expressed in the global/model frame this subtraction is done in the wrong basis. Rotate the centroid offset into the IB/local frame with the same inverse_rotation before subtracting to ensure the offset is applied consistently in the same coordinate system. [logic error]
Severity Level: Critical 🚨
- ❌ 3D airfoil marker placement incorrect in s_ib_3D_airfoil.
- ⚠️ 3D pitching/rotating airfoil simulations show wrong geometry.| xyz_local = xyz_local - patch_ib(patch_id)%centroid_offset ! airfoils are a patch that require a centroid offset | |
| xyz_local = xyz_local - matmul(inverse_rotation, patch_ib(patch_id)%centroid_offset) ! rotate centroid_offset into IB/local frame before subtracting |
Steps of Reproduction ✅
1. Prepare a 3D case with an airfoil patch (patch_ib(... )%geometry == 11) so
s_apply_ib_patches calls s_ib_3D_airfoil during marker generation.
2. Set patch_ib(patch_id)%centroid_offset to a non-zero vector and set non-zero rotation
angles so rotation_matrix_inverse is non-trivial; run to the IB marker generation step.
3. In s_ib_3D_airfoil the code computes xyz_local and applies inverse_rotation at
src/common/m_ib_patches.fpp:435-436, then subtracts patch_ib(patch_id)%centroid_offset at
line 437 without rotating that offset into the local frame.
4. As a result the z/y/x cell-inclusion tests (for example the z_min/z_max check at the
surrounding block) and the later assignments to ib_markers_sf(i,j,l) are done with an
inconsistent offset and the 3D airfoil appears shifted. Reproduces when centroid_offset ≠
0 and rotation ≠ identity.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/m_ib_patches.fpp
**Line:** 437:437
**Comment:**
*Logic Error: The 3D airfoil routine subtracts `patch_ib(patch_id)%centroid_offset` from `xyz_local` after applying `inverse_rotation`; if `centroid_offset` is expressed in the global/model frame this subtraction is done in the wrong basis. Rotate the centroid offset into the IB/local frame with the same `inverse_rotation` before subtracting to ensure the offset is applied consistently in the same coordinate system.
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.
src/simulation/m_ibm.fpp
Outdated
| call s_mpi_allreduce_sum(center_of_mass(2), center_of_mass(2)) | ||
| call s_mpi_allreduce_sum(center_of_mass(3), center_of_mass(3)) | ||
| call s_mpi_allreduce_integer_sum(num_cells_local, num_cells) | ||
| center_of_mass = center_of_mass / real(num_cells_local, wp) |
There was a problem hiding this comment.
✅ Suggestion: Logic error: the code divides the reduced (global) sum of coordinates by the local rank cell count (num_cells_local) instead of the global cell count (num_cells), which yields an incorrect centroid and can cause a division-by-zero when the local rank has zero cells; change the divisor to the globally reduced num_cells and guard against zero. [logic error]
Severity Level: Critical 🚨
- ❌ Centroid miscomputed for airfoil/STL IBs (s_compute_centroid_offset)
- ❌ Possible runtime crash on ranks with zero local IB cells
- ⚠️ Affects s_ibm_setup and subsequent IB motion calculations
- ⚠️ Breaks pitching-airfoil test cases (user-reported)| center_of_mass = center_of_mass / real(num_cells_local, wp) | |
| if (num_cells == 0) then | |
| ! No cells globally for this IB -- leave offsets at zero and exit safely | |
| patch_ib(ib_marker)%centroid_offset(:) = [0._wp, 0._wp, 0._wp] | |
| return | |
| end if | |
| center_of_mass = center_of_mass / real(num_cells, wp) |
Steps of Reproduction ✅
1. Build and run the code with MPI (>=2 ranks) and a case that registers an IB of geometry
type handled here (geometry == 4/5/11/12). The initialization path runs s_ibm_setup
(src/simulation/m_ibm.fpp: around lines 93-105) which calls s_compute_centroid_offset.
2. Ensure the IB lies entirely within one MPI rank's subdomain such that other ranks have
zero cells for that IB. During s_compute_centroid_offset (subroutine start at
src/simulation/m_ibm.fpp: ~1086) each rank counts local cells into num_cells_local and
sums coordinates into center_of_mass.
3. The code calls s_mpi_allreduce_integer_sum(num_cells_local, num_cells) (line 1120)
which returns global num_cells, but then divides by the local num_cells_local at line
1121. If the local rank had num_cells_local == 0, this division triggers a divide-by-zero
on that rank causing a crash; otherwise the centroid is computed incorrectly (using local
count instead of global).
4. Observe either a floating-point exception / program abort on ranks with zero local
cells or an incorrect centroid value being written to patch_ib(...)%x_centroid etc.
Confirm by inspecting outputs written after s_compute_centroid_offset or by adding a debug
print near lines 1115-1121.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 1121:1121
**Comment:**
*Logic Error: Logic error: the code divides the reduced (global) sum of coordinates by the local rank cell count (`num_cells_local`) instead of the global cell count (`num_cells`), which yields an incorrect centroid and can cause a division-by-zero when the local rank has zero cells; change the divisor to the globally reduced `num_cells` and guard against zero.
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.There was a problem hiding this comment.
This comment actually doesn't stink
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pre_process/m_check_ib_patches.fpp (1)
65-70: Add validation for ellipse geometry and update error message to include geometry 6.The ellipse patch (geometry == 6) branch lacks parameter validation that exists for all other geometry types. Additionally, the error message on line 70 must be updated to include geometry 6 in the list of valid geometries to match the explicit handling at line 65.
🔧 Suggested fixes
- Update the error message to include geometry 6:
call s_prohibit_abort("Invalid IB patch", & "patch_ib("//trim(iStr)//")%geometry must be "// & - "2-4, 8-10, 11 or 12.") + "2-6, 8-10, 11 or 12.")
- Add a validation subroutine for ellipse geometry and call it:
else if (patch_ib(i)%geometry == 6) then call s_check_ellipse_ib_patch_geometry(i)Then implement
s_check_ellipse_ib_patch_geometryto validate centroid and radii parameters similarly to the circle patch validation.src/simulation/m_time_steppers.fpp (1)
629-643: The@:mib_analytical()macro at line 631 is undefined and will cause a preprocessing error.The macro is invoked for 1-way coupling (
moving_ibm == 1) but is not defined incase.fpp,macros.fpp, or any other.fppfile in the codebase. Only an emptyanalytical()macro (without themib_prefix) exists incase.fpp.The macro must be properly defined to specify how analytic velocities are applied to the IB patches in 1-way coupling mode. Add the implementation to
case.fppor the appropriate macro file.
🤖 Fix all issues with AI agents
In `@examples/2D_ibm_ellipse/case.py`:
- Around line 84-90: Update the misleading comment "Cylinder Immersed Boundary"
to describe the actual geometry being configured (an ellipse) where the patch
keys like "patch_ib(1)%geometry" are set to 6; change the comment near the block
that sets "patch_ib(1)%geometry", "patch_ib(1)%x_centroid",
"patch_ib(1)%y_centroid", "patch_ib(1)%length_x", "patch_ib(1)%length_y", and
"patch_ib(1)%slip" so it accurately states "Ellipse Immersed Boundary" (or
equivalent) to match geometry=6.
In `@src/common/m_compute_levelset.fpp`:
- Line 380: The inline comment on the line assigning normal_vector(2) contains a
typo "transofmration"; update the comment text to "transformation" in the same
statement so it reads "... via the coordinate transformation method" while
leaving the code (normal_vector(2) = normal_vector(2) *
(ellipse_coeffs(1)/ellipse_coeffs(2))**2._wp) and surrounding whitespace
unchanged.
In `@src/common/m_ib_patches.fpp`:
- Around line 789-792: The comment in the m_ib_patches module that reads
"Checking whether the rectangle covers..." is incorrect for the ellipse-handling
subroutine; update the comment to say "ellipse" (and adjust surrounding phrasing
if needed, e.g., "ellipse covers a particular cell") so it accurately reflects
the geometry handled by the subroutine in this file.
In `@src/simulation/m_ibm.fpp`:
- Around line 1115-1122: The center_of_mass calculation divides by the local
count; after calling s_mpi_allreduce_sum(center_of_mass) and
s_mpi_allreduce_integer_sum(num_cells_local, num_cells) replace the division by
real(num_cells_local, wp) with real(num_cells, wp) so the globally reduced
center_of_mass is normalized by the global cell count (ensure you use the
reduced integer num_cells, not num_cells_local, in the division and keep the wp
real-kind conversion).
🧹 Nitpick comments (1)
toolchain/mfc/case.py (1)
223-256: Consider bindingpidexplicitly to avoid closure capture issue.The static analyzer flagged that
rhs_replacecaptures the loop variablepidby reference. While this is safe here since the function is called immediately within the same iteration, it's a potential maintenance hazard if the code is refactored.♻️ Optional fix using default argument binding
for pid, items in ib_patches.items(): # function that defines how we will replace variable names with # values from the case file - def rhs_replace(match): + def rhs_replace(match, pid=pid): return { 'x': 'x_cc(i)', 'y': 'y_cc(j)', 'z': 'z_cc(k)', 't': 'mytime', 'r': f'patch_ib({pid})%radius', 'e' : f'{math.e}', }.get(match.group(), match.group())
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/2D_ibm_ellipse/case.pysrc/common/m_compute_levelset.fppsrc/common/m_derived_types.fppsrc/common/m_ib_patches.fppsrc/pre_process/m_check_ib_patches.fppsrc/pre_process/m_global_parameters.fppsrc/simulation/m_ibm.fppsrc/simulation/m_time_steppers.fpptoolchain/mfc/case.pytoolchain/mfc/run/case_dicts.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!!@paramand `!! `@returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/common/m_derived_types.fppsrc/pre_process/m_check_ib_patches.fppsrc/simulation/m_ibm.fppsrc/simulation/m_time_steppers.fppsrc/common/m_compute_levelset.fppsrc/common/m_ib_patches.fppsrc/pre_process/m_global_parameters.fpp
src/**/*.fpp
📄 CodeRabbit inference engine (.cursor/rules/mfc-agent-rules.mdc)
src/**/*.fpp: Use.fppfile extension for Fypp preprocessed files; CMake transpiles them to.f90
Start module files with Fypp include for macros:#:include 'macros.fpp'
Use the fyppASSERTmacro for validating conditions:@:ASSERT(predicate, message)
Use fypp macro@:ALLOCATE(var1, var2)for device-aware allocation instead of standard Fortranallocate
Use fypp macro@:DEALLOCATE(var1, var2)for device-aware deallocation instead of standard Fortrandeallocate
Files:
src/common/m_derived_types.fppsrc/pre_process/m_check_ib_patches.fppsrc/simulation/m_ibm.fppsrc/simulation/m_time_steppers.fppsrc/common/m_compute_levelset.fppsrc/common/m_ib_patches.fppsrc/pre_process/m_global_parameters.fpp
src/simulation/**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/simulation/**/*.{fpp,f90}: Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Avoid stop/error stop inside GPU device code
Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
src/simulation/**/*.{fpp,f90}: Mark GPU-callable helpers with$:GPU_ROUTINE(function_name='...', parallelism='[seq]')immediately after declaration
Do not use OpenACC or OpenMP directives directly; use Fypp macros fromsrc/common/include/parallel_macros.fppinstead
Wrap tight loops with$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')macro; addcollapse=nfor safe nested loop merging
Declare loop-local variables withprivate='[...]'in GPU parallel loop macros
Allocate large arrays withmanagedor move them into a persistent$:GPU_ENTER_DATA(...)region at start-up
Do not placestoporerror stopinside device code
Files:
src/simulation/m_ibm.fppsrc/simulation/m_time_steppers.fpp
🧠 Learnings (7)
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Applied to files:
src/simulation/m_ibm.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Allocate large arrays with `managed` or move them into a persistent `$:GPU_ENTER_DATA(...)` region at start-up
Applied to files:
src/simulation/m_ibm.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight loops with `$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')` macro; add `collapse=n` for safe nested loop merging
Applied to files:
src/simulation/m_ibm.fppsrc/simulation/m_time_steppers.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Do not use OpenACC or OpenMP directives directly; use Fypp macros from `src/common/include/parallel_macros.fpp` instead
Applied to files:
src/simulation/m_time_steppers.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
Applied to files:
src/simulation/m_time_steppers.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Applied to files:
src/simulation/m_time_steppers.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `private` declaration followed by explicit `public` exports in modules
Applied to files:
src/common/m_compute_levelset.fpp
🧬 Code graph analysis (3)
src/simulation/m_ibm.fpp (1)
toolchain/mfc/test/cases.py (1)
alter_ib(377-422)
examples/2D_ibm_ellipse/case.py (1)
toolchain/mfc/test/cases.py (8)
alter_ib(377-422)alter_3d(322-365)alter_2d(293-320)alter_hypoelasticity(590-652)alter_num_fluids(224-291)alter_mixlayer_perturb(686-717)alter_viscosity(788-839)alter_bc_patches(908-955)
toolchain/mfc/case.py (1)
toolchain/mfc/common.py (1)
is_number(190-201)
🪛 GitHub Actions: Spell Check
src/common/m_compute_levelset.fpp
[error] 380-380: Typos found. 'transofmration' should be 'transformation'.
🪛 Ruff (0.14.11)
toolchain/mfc/case.py
232-232: Function definition does not bind loop variable pid
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (20)
toolchain/mfc/run/case_dicts.py (1)
368-371: LGTM! Analytic type support for IB velocity parameters.The change to
ParamType.REAL.analytic()forvelandangular_velcorrectly enables time-parameterized motion expressions for moving immersed boundaries, consistent with how other analytic initial conditions are handled elsewhere in the codebase.toolchain/mfc/case.py (5)
15-18: LGTM! Well-structured constant for MIB analytic variables.The
MIBM_ANALYTIC_VARSlist correctly enumerates all velocity and angular velocity components that can be analytically defined for moving immersed boundaries.
101-111: LGTM! Proper regex escaping for variable matching.Good use of
re.escape(variable)to handle parentheses in variable names likevel(1). The method correctly mirrors the pattern established by__is_ic_analytical.
350-358: LGTM! Clean refactoring for FPP generation.The separation of
__get_analytic_ic_fppand__get_analytic_mib_fppwith proper concatenation in__get_sim_fppensures both IC and MIB analytic macros are available during simulation compilation.
54-57: LGTM! Proper handling of MIB analytic expressions in input generation.Correctly extends the analytic placeholder logic to MIB parameters, ensuring string expressions are replaced with
0d0in the.inpfile while the actual expressions are handled via generated Fypp macros.
258-265: No action needed —mytimeis properly available in scope.The generated
#:def mib_analytical()macro correctly maps the time variablettomytime, which is defined as a module-level variable inm_global_parameters.fppand is initialized and maintained throughout the simulation lifecycle. It is accessible wherever the macro is expanded.src/common/m_derived_types.fpp (1)
299-299: LGTM! Well-documented centroid offset field.The
centroid_offsetfield correctly addresses issue#1106by allowing proper tracking of the center of mass offset for asymmetric immersed boundaries like airfoils. The field type and dimensioning are appropriate.src/pre_process/m_global_parameters.fpp (1)
565-565: LGTM! Proper default initialization for centroid_offset.The initialization to
0._wpis consistent with other array initializations in the IB patch defaults and ensures a clean starting state before the actual centroid offset is computed during simulation setup.examples/2D_ibm_ellipse/case.py (1)
1-97: LGTM - New 2D ellipse example case.The case file correctly demonstrates the new ellipse IB patch (geometry=6) with appropriate domain parameters, boundary conditions, and fluid properties for viscous flow simulation.
src/simulation/m_time_steppers.fpp (1)
5-6: LGTM - Include case.fpp for analytic MIB macros.Adding the include enables the
@:mib_analytical()macro for prescribing analytic velocities in 1-way MIBM coupling.src/simulation/m_ibm.fpp (2)
96-104: LGTM - Added centroid offset computation in IBM setup.The call to
s_compute_centroid_offset(i)is correctly placed after the rotation matrix update and before GPU data transfer, enabling proper centroid calculations for airfoil and STL geometries.
1103-1113: Use the more directif (p > 0)check for consistency with the rest of the codebase. Line 1109 should beif (p > 0)instead ofif (num_dims == 3), matching the pattern used elsewhere (e.g., lines 224–227) and avoiding reliance on a global parameter that should inherently correspond to the grid dimensions.src/common/m_compute_levelset.fpp (4)
26-27: LGTM - Added ellipse levelset to public exports.The new
s_ellipse_levelsetsubroutine is correctly added to the module's public interface.
97-97: LGTM - Centroid offset adjustment for 2D airfoil levelset.Correctly subtracts
centroid_offsetfrom local coordinates after rotation, aligning the airfoil's geometric center with its true center of mass for proper levelset computation.
193-193: LGTM - Centroid offset adjustment for 3D airfoil levelset.Consistent with the 2D implementation, correctly applies centroid offset in the local IB frame.
342-396: Resolve the sign discrepancy between the formula comment and implementation on line 390.The comment on line 389 states to compute
[ -B + sqrt(B^2 - 4AC) ] / 2A(the plus root), but the code on line 390 implements-0.5_wp * (-quadratic_coeffs(2) + sqrt(...)) / quadratic_coeffs(1), which mathematically evaluates to(B - sqrt(...)) / (2A)(the minus root with a sign flip). Verify whether this negation is intentional (e.g., for inward-facing signed distance) and update the comment to reflect the actual formula being computed.Regarding the other points: centroid_offset is correctly omitted since ellipses have a well-defined geometric center (unlike airfoils which require explicit offset correction, as noted in s_airfoil_levelset). Minor style: replace
**2._wpexponents on lines 380, 387 with**2per coding guidelines.src/common/m_ib_patches.fpp (4)
122-125: LGTM - Added ellipse geometry dispatch in 2D IB patch application.The new geometry type 6 for ellipse is correctly routed to both the marker function (
s_ib_ellipse) and levelset computation (s_ellipse_levelset).
282-282: LGTM - Centroid offset for 2D airfoil patch markers.Correctly applies centroid offset in the local IB frame, consistent with the levelset computation in
m_compute_levelset.fpp.
437-437: LGTM - Centroid offset for 3D airfoil patch markers.Consistent implementation with the 2D version.
771-810: LGTM - New ellipse IB patch subroutine.The implementation correctly:
- Extracts ellipse semi-axes from
length_x/2andlength_y/2- Transforms coordinates to local IB frame using rotation matrix
- Uses the standard ellipse equation
(x/a)² + (y/b)² ≤ 1for interior check- Follows the same pattern as other 2D patch subroutines
The GPU parallel loop is properly configured with appropriate private variables and data movement.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 10 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/simulation/m_ibm.fpp">
<violation number="1" location="src/simulation/m_ibm.fpp:1121">
P1: After the MPI reductions, the centroid is divided by `num_cells_local` (the pre-reduction local count) instead of the globally reduced `num_cells`, which can produce NaNs or incorrect centers of mass. Divide by the global count returned by `s_mpi_allreduce_integer_sum` (and guard against zero) to compute a valid centroid.</violation>
</file>
<file name="src/pre_process/m_check_ib_patches.fpp">
<violation number="1" location="src/pre_process/m_check_ib_patches.fpp:65">
P2: Geometry 6 IB patches bypass all validation: the new branch just prints “Ellipse Patch” and never enforces required parameters (centroid, radii, dimensionality). This lets malformed ellipse IB definitions pass unchecked and can cause runtime failures when s_ib_ellipse later uses unset data. Replace the print with proper validation mirroring other geometries.</violation>
</file>
<file name="toolchain/mfc/run/case_dicts.py">
<violation number="1" location="toolchain/mfc/run/case_dicts.py:369">
P1: Assigning `ParamType.REAL.analytic()` (which returns a plain dict) directly into `SIMULATION` is inconsistent with other entries that use `ParamType` Enum members. Downstream code that builds `_properties = {k: v.value for k, v in ALL.items()}` will fail with an `AttributeError` because dicts don't have a `.value` attribute. Use `ParamType.REAL` instead to maintain consistency.</violation>
<violation number="2" location="toolchain/mfc/run/case_dicts.py:371">
P1: Same issue as with `vel`: assigning `ParamType.REAL.analytic()` (a plain dict) will cause `AttributeError` when building `_properties`. Use `ParamType.REAL` instead.</violation>
</file>
<file name="src/common/m_compute_levelset.fpp">
<violation number="1" location="src/common/m_compute_levelset.fpp:377">
P1: levelset and normal fields are never set for cells outside the ellipse, leaving undefined geometry data for most of the domain. Add an `else` branch that computes the signed distance/normal outside the ellipse instead of skipping the assignment.</violation>
</file>
<file name="toolchain/mfc/case.py">
<violation number="1" location="toolchain/mfc/case.py:227">
P1: Analytic IB codegen replaces `x`, `y`, `z` with grid-center arrays using undefined indices (`j`, `k`) inside the time-stepper loop, causing compilation failures or wrong values when users reference spatial variables.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/common/m_ib_patches.fpp`:
- Line 781: The inline comment "! Transferring the rectangle's centroid and
length information" is a copy-paste error; update it to refer to the ellipse
instead (e.g., "! Transferring the ellipse's centroid and axes/length
information") so it accurately describes the code handling ellipse centroid and
length/axes values; locate and change this exact comment string in
m_ib_patches.fpp (search for "Transferring the rectangle's centroid") and
replace "rectangle" with "ellipse" (optionally adjust "length" to "axes" for
clarity).
♻️ Duplicate comments (2)
src/common/m_ib_patches.fpp (1)
788-791: Fix copy-paste comment: "rectangle" should be "ellipse".📝 Proposed fix
- ! Checking whether the rectangle covers a particular cell in the + ! Checking whether the ellipse covers a particular cell in the ! domain and verifying whether the current patch has the permission ! to write to that cell. If both queries check out, the primitive ! variables of the current patch are assigned to this cell.src/common/m_compute_levelset.fpp (1)
376-391: Level set only computed inside ellipse - exterior cells remain uninitialized.The condition at line 377 restricts computation to points inside the ellipse. Cells outside will retain their initial values (likely zero from the caller's initialization), which may cause incorrect ghost point interpolation for cells near but outside the ellipse boundary.
Compare to
s_circle_levelset(lines 49-62) which computesdist - radiusfor ALL grid points, providing a valid signed distance everywhere.🐛 Proposed fix: Compute signed distance for all points
For points outside the ellipse, you could use an algebraic distance approximation:
- if ((xy_local(1)/ellipse_coeffs(1))**2 + (xy_local(2)/ellipse_coeffs(2))**2 <= 1._wp) then + ! Algebraic distance approximation valid everywhere + levelset%sf(i, j, 0, ib_patch_id) = sqrt(ellipse_coeffs(1) * ellipse_coeffs(2)) * & + (sqrt((xy_local(1)/ellipse_coeffs(1))**2 + (xy_local(2)/ellipse_coeffs(2))**2) - 1._wp) + ! Normal vector computation only valid inside/near surface + if ((xy_local(1)/ellipse_coeffs(1))**2 + (xy_local(2)/ellipse_coeffs(2))**2 <= 1._wp) then normal_vector = xy_local normal_vector(2) = normal_vector(2)*(ellipse_coeffs(1)/ellipse_coeffs(2))**2._wp normal_vector = normal_vector/sqrt(dot_product(normal_vector, normal_vector)) levelset_norm%sf(i, j, 0, ib_patch_id, :) = matmul(rotation, normal_vector) - - quadratic_coeffs(1) = ... - ... - levelset%sf(i, j, 0, ib_patch_id) = ... end ifAlternatively, compute the exact closest-point-on-ellipse distance for exterior points, though this requires iterative root finding.
🧹 Nitpick comments (2)
src/simulation/m_ibm.fpp (1)
1115-1121: Previous critical bug is fixed, but consider guarding against zero cell count.The division now correctly uses the globally reduced
num_cellsinstead ofnum_cells_local. However, if an IB patch has no cells (e.g., misconfigured geometry),num_cellscould be zero, causing a division-by-zero crash.🛡️ Optional: Add zero-guard for robustness
call s_mpi_allreduce_sum(center_of_mass(1), center_of_mass(1)) call s_mpi_allreduce_sum(center_of_mass(2), center_of_mass(2)) call s_mpi_allreduce_sum(center_of_mass(3), center_of_mass(3)) call s_mpi_allreduce_integer_sum(num_cells_local, num_cells) + if (num_cells == 0) then + call s_mpi_abort("s_compute_centroid_offset: No cells found for IB patch") + end if center_of_mass = center_of_mass/real(num_cells, wp)src/common/m_compute_levelset.fpp (1)
356-357: Remove unused variableskandidx.These variables are declared but never used in the subroutine.
♻️ Proposed fix
- integer :: i, j, k !< Loop index variables - integer :: idx !< Shortest path direction indicator + integer :: i, j !< Loop index variables
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/common/m_compute_levelset.fppsrc/common/m_ib_patches.fppsrc/simulation/m_ibm.fpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!!@paramand `!! `@returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/common/m_ib_patches.fppsrc/simulation/m_ibm.fppsrc/common/m_compute_levelset.fpp
src/**/*.fpp
📄 CodeRabbit inference engine (.cursor/rules/mfc-agent-rules.mdc)
src/**/*.fpp: Use.fppfile extension for Fypp preprocessed files; CMake transpiles them to.f90
Start module files with Fypp include for macros:#:include 'macros.fpp'
Use the fyppASSERTmacro for validating conditions:@:ASSERT(predicate, message)
Use fypp macro@:ALLOCATE(var1, var2)for device-aware allocation instead of standard Fortranallocate
Use fypp macro@:DEALLOCATE(var1, var2)for device-aware deallocation instead of standard Fortrandeallocate
Files:
src/common/m_ib_patches.fppsrc/simulation/m_ibm.fppsrc/common/m_compute_levelset.fpp
src/simulation/**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/simulation/**/*.{fpp,f90}: Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Avoid stop/error stop inside GPU device code
Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
src/simulation/**/*.{fpp,f90}: Mark GPU-callable helpers with$:GPU_ROUTINE(function_name='...', parallelism='[seq]')immediately after declaration
Do not use OpenACC or OpenMP directives directly; use Fypp macros fromsrc/common/include/parallel_macros.fppinstead
Wrap tight loops with$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')macro; addcollapse=nfor safe nested loop merging
Declare loop-local variables withprivate='[...]'in GPU parallel loop macros
Allocate large arrays withmanagedor move them into a persistent$:GPU_ENTER_DATA(...)region at start-up
Do not placestoporerror stopinside device code
Files:
src/simulation/m_ibm.fpp
🧠 Learnings (4)
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Applied to files:
src/simulation/m_ibm.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Allocate large arrays with `managed` or move them into a persistent `$:GPU_ENTER_DATA(...)` region at start-up
Applied to files:
src/simulation/m_ibm.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight loops with `$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')` macro; add `collapse=n` for safe nested loop merging
Applied to files:
src/simulation/m_ibm.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `private` declaration followed by explicit `public` exports in modules
Applied to files:
src/common/m_compute_levelset.fpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Build & Publish
🔇 Additional comments (8)
src/simulation/m_ibm.fpp (2)
1084-1136: LGTM! Well-structured centroid offset computation.The subroutine correctly:
- Targets only geometry types requiring offset computation (airfoils, STL models)
- Uses MPI reduction for distributed computation
- Transforms the offset back to local IB coordinates for consistent usage
- Initializes offset to zero for inapplicable geometries
96-105: LGTM! Correct integration of centroid offset computation.The call ordering is correct: rotation matrix is computed first (required for the offset rotation at line 1131), and the centroid offset is computed before the GPU data transfer.
src/common/m_compute_levelset.fpp (2)
95-97: LGTM! Correct coordinate frame for centroid offset subtraction.The
centroid_offsetis stored in local IB coordinates (transformed at m_ibm.fpp:1131), and here it's correctly subtracted afterxy_localhas been rotated into the same local frame.
191-193: LGTM! Consistent with 2D airfoil implementation.src/common/m_ib_patches.fpp (4)
280-282: LGTM! Centroid offset correctly applied in local coordinates.The
centroid_offsetis pre-rotated into local IB coordinates during initialization (m_ibm.fpp:1131). Here it's correctly subtracted afterxy_localis transformed to the same local frame. Both vectors share the same coordinate basis.
435-437: LGTM! Consistent centroid offset handling for 3D airfoil.
770-809: LGTM! Clean ellipse IB marker implementation.The subroutine correctly:
- Extracts ellipse parameters from patch data
- Transforms coordinates to local IB frame
- Uses standard ellipse membership test
(x/a)² + (y/b)² ≤ 1
122-124: LGTM! Correct dispatch for ellipse geometry.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
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:L
|
CodeAnt AI Incremental review completed. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
+ Coverage 44.06% 44.16% +0.10%
==========================================
Files 71 71
Lines 20379 20412 +33
Branches 1989 1992 +3
==========================================
+ Hits 8980 9015 +35
+ Misses 10254 10250 -4
- Partials 1145 1147 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codeant-ai
bot
added
size:L
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/simulation/m_ibm.fpp`:
- Around line 1170-1171: The two variable assignments center_of_mass_local and
num_cells_local in the same if-block have inconsistent indentation causing the
pipeline failure; align the indentation so both statements use the same
indentation level (match the indent used for num_cells_local or
center_of_mass_local) within the function or block in src/simulation/m_ibm.fpp
to ensure consistent formatting.
- Around line 1186-1193: After the MPI reductions (s_mpi_allreduce_sum on
center_of_mass_local and s_mpi_allreduce_integer_sum on num_cells) add a guard
to avoid dividing by zero: check if num_cells == 0 and in that case set
center_of_mass = 0.0_wp (or another defined safe default) and skip the division;
otherwise perform center_of_mass = center_of_mass/real(num_cells, wp). This
change touches the variables center_of_mass, center_of_mass_local,
num_cells_local, num_cells and the reduction sequence (s_mpi_allreduce_sum /
s_mpi_allreduce_integer_sum) so place the conditional immediately after the
reductions and before the division.
♻️ Duplicate comments (1)
src/simulation/m_ibm.fpp (1)
98-107: Add guard against division by zero ins_compute_centroid_offsetwhen no cells are found for the IB patch.The call placement at line 106 is correct—
ib_markers%sfis populated befores_ibm_setup()is invoked (from the restart file at line 440 inm_start_up.fpp). However, line 1192 divides bynum_cellswithout checking if it is zero. If a NACA airfoil or STL geometry has no cells assigned across all ranks, the MPI reduction leavesnum_cells = 0, causing NaN values to propagate tocenter_of_mass.Add a guard after the MPI reduction:
call s_mpi_allreduce_integer_sum(num_cells_local, num_cells) if (num_cells > 0) then center_of_mass = center_of_mass/real(num_cells, wp) else center_of_mass(:) = [0._wp, 0._wp, 0._wp] end if
…b.com:danieljvickers/MFC into analytic-mibm-velocities-and-airfoil-centroid
|
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:L
|
CodeAnt AI Incremental review completed. |
|
Just a note: I know that this passed tests, but I want to make a few more changes before this is merged to clean up some code. |
|
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:L
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/simulation/m_time_steppers.fpp`:
- Around line 791-804: The code currently calls s_compute_ib_forces(...) inside
the per-IB loop whenever patch_ib(i)%moving_ibm == 2, causing redundant
full-domain force computations; instead, before the do i = 1, num_ibs loop
detect if any patch_ib(:)%moving_ibm == 2 (e.g., a boolean flag set by scanning
patch_ib) and if so call s_compute_ib_forces(q_prim_vf, 1._wp/fluid_pp(1)%Re(1))
once for the stage, then inside the loop remove the s_compute_ib_forces call and
only perform the per-IB updates to patch_ib(i)%vel, patch_ib(i)%angular_vel,
call s_compute_moment_of_inertia(i, ...) and the subsequent division by
patch_ib(i)%moment.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/simulation/m_ibm.fpp">
<violation number="1" location="src/simulation/m_ibm.fpp:1053">
P1: Pressure-gradient accumulation overwrites Y/Z components with the X component, producing incorrect force/torque directions. Each component should accumulate from its own value.</violation>
<violation number="2" location="src/simulation/m_ibm.fpp:1073">
P1: Dynamic viscosity uses an uninitialized index (`l`) and misses the current `i` index, so it reads the wrong cell (undefined in Fortran). This can corrupt viscous force computations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/simulation/m_ibm.fpp`:
- Around line 1070-1074: The dynamic_viscosity accumulation uses
q_cons_vf(...%sf(j, k, l)) but the loop is over (i, j, k) so the index l is
wrong; update the access to use the correct i index (q_cons_vf(fluid_idx + advxb
- 1)%sf(i, j, k)) so dynamic_viscosity is computed from the current (i,j,k) cell
when summing over fluids and using fluid_pp%Re within the loop in m_ibm.fpp.
- Around line 1049-1059: The y and z components of the pressure-force
accumulator are accidentally using local_force_contribution(1) on the right-hand
side, which mixes axes; in the block that computes local_force_contribution(2)
and, inside the num_dims==3 branch, local_force_contribution(3), change the RHS
to reference the same component being updated (use local_force_contribution(2)
for the y update and local_force_contribution(3) for the z update) so each axis
subtracts its own finite-difference pressure gradient (retain use of
q_prim_vf(E_idx + fluid_idx)%sf(...) and the existing dx, dy, dz and cell_volume
logic).
♻️ Duplicate comments (3)
src/simulation/m_ibm.fpp (2)
98-107: Move centroid-offset computation after IB markers are populated.
s_compute_centroid_offsetscansib_markers%sf, but this call happens befores_apply_ib_patches/s_populate_ib_buffers, so it runs on empty markers and does a full-domain scan per IB with zero counts. Please move it after the markers are populated.🔧 Suggested move
- call s_compute_centroid_offset(i)call s_populate_ib_buffers() + do i = 1, num_ibs + call s_compute_centroid_offset(i) + end do
1193-1200: Guard against zero cells before dividing.
Ifnum_cells == 0across all ranks, this divides by zero and propagates NaNs into centroid placement.🛡️ Suggested guard
call s_mpi_allreduce_integer_sum(num_cells_local, num_cells) - center_of_mass = center_of_mass/real(num_cells, wp) + if (num_cells == 0) then + patch_ib(ib_marker)%centroid_offset(:) = [0._wp, 0._wp, 0._wp] + return + end if + center_of_mass = center_of_mass/real(num_cells, wp)src/simulation/m_time_steppers.fpp (1)
772-805: Compute IB forces once per stage, not once per IB.
s_compute_ib_forcesalready sweeps the full domain for all IBs; calling it inside the per-IB loop repeats that work for everymoving_ibm == 2patch. Please move the call outside the loop and guard it withany(patch_ib(:)%moving_ibm == 2).♻️ Suggested restructuring
+ if (any(patch_ib(:)%moving_ibm == 2)) then + call s_compute_ib_forces(q_prim_vf, fluid_pp) + end if do i = 1, num_ibs @@ - else if (patch_ib(i)%moving_ibm == 2) then ! if we are using two-way coupling, apply force and torque - ! compute the force and torque on the IB from the fluid - call s_compute_ib_forces(q_prim_vf, fluid_pp) + else if (patch_ib(i)%moving_ibm == 2) then ! if we are using two-way coupling, apply force and torque
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/simulation/m_ibm.fpp">
<violation number="1" location="src/simulation/m_ibm.fpp:1073">
P2: The dynamic viscosity computation indexes q_prim_vf with (j, k, l) even though l is not initialized in this loop. This can read the wrong cell (or an undefined index), producing incorrect forces/torques. Use the current cell indices (i, j, k).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Ok, I have addressed all AI review comments and finished the basic updates that I also wanted to run in this PR. Once this passes the test suite again, it will be ready to merge |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/simulation/m_ibm.fpp`:
- Around line 1049-1061: The pressure gradient is being computed per-fluid using
q_prim_vf(E_idx + fluid_idx) which is wrong because E_idx is the single shared
pressure index; change the code to compute the pressure gradient using
q_prim_vf(E_idx)%sf(...) only (not E_idx + fluid_idx). Either move the gradient
computation for local_force_contribution(1..3) outside the fluid loop or compute
a local pressure_gradient variable from q_prim_vf(E_idx)%sf(...) and then add
that same contribution to each fluid's local_force_contribution inside the loop;
keep the existing cell_volume and num_dims handling unchanged.
🧹 Nitpick comments (1)
src/simulation/m_ibm.fpp (1)
1028-1028: Remove unused variablevel.The variable
velis declared but never used in this subroutine.♻️ Proposed fix
- real(wp), dimension(1:3) :: local_force_contribution, radial_vector, local_torque_contribution, vel + real(wp), dimension(1:3) :: local_force_contribution, radial_vector, local_torque_contribution
…iles for some reason
2 participants
User description
User description
Description
Adds a numeric method of computing the centroid of IBs where the center of mass is not well posed. In particular, I added hooks for STL and airfiol IBs.
This PR also includes support for parameterized IB motion. It allows one to analytically describe the velocity and rotation rate of any IB experiencing 1-way coupling.
Both of these features have been tested with IB's the move and rotate using the new analytic parameterization. I have also tested with an airfoil rotating about the correct center of mass. Videos available in my slides.
Fixes #1105
Fixes #1106
In addition to these issues being fixed, I also resolved a separate bug that is related in getting the 2-way force calculation to work with multi-fluid flow. I also included some minor refactoring to clean up the time stepper subroutine.
Type of change
Please delete options that are not relevant.
Scope
That goal was enabling the ability to run a pitching airfoil case, which oscillates in time.
How Has This Been Tested?
The best configuration can be summarized with this test case file for the pitching airfoil:
Checklist
docs/)examples/that demonstrate my new feature performing as expected.They run to completion and demonstrate "interesting physics"
./mfc.sh formatbefore committing my codeIf your code changes any code source files (anything in
src/simulation)To make sure the code is performing as expected on GPU devices, I have:
nvtxranges so that they can be identified in profiles./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR./mfc.sh run XXXX --gpu -t simulation --rsys --hip-trace, and have attached the output file and plain text results to this PR.PR Type
Enhancement
Description
Add ellipse immersed boundary patch geometry with levelset computation
Implement centroid offset calculation for complex IB shapes (airfoils, STL)
Support analytical parameterization of moving IB velocities and rotation rates
Enable 1-way coupling with analytically-defined IB motion in time-stepping
Diagram Walkthrough
File Walkthrough
9 files
Add ellipse levelset computation and centroid offsetAdd centroid offset field to IB patch typeAdd ellipse IB patch initialization and marker setupAdd validation for ellipse IB patch geometryInitialize centroid offset to zero for all patchesAdd centroid offset computation and 1-way coupling supportIntegrate analytical velocity parameterization for 1-way couplingAdd code generation for analytical moving IB velocitiesMark IB velocity and angular velocity as analytically-parameterizable1 files
Add example case configuration for ellipse IB simulationCodeAnt-AI Description
Support analytic motion for moving immersed boundaries and correct centroid for airfoils/STL patches
What Changed
Impact
✅ Correct airfoil pitch center during rotation✅ Predictable user-defined IB motions for 1-way coupling✅ Fewer simulation setup errors for odd-shaped IBs💡 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
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.