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 centroid_offset handling for IB patches, applies centroid corrections in airfoil levelsets, introduces ellipse patch geometry, refactors IBM force computation for per-fluid viscosities and centroid computation, and centralizes IB state propagation with a new RK-stage helper. Changes
sequenceDiagram
rect rgba(200,200,255,0.5)
participant TS as TimeStepper
end
rect rgba(200,255,200,0.5)
participant IBM as IBM Module (m_ibm)
participant PCH as Patch Data (patch_ib)
end
rect rgba(255,200,200,0.5)
participant MPI as MPI/Domain
participant GRID as Grid/Levelset
end
TS->>IBM: call s_propagate_immersed_boundaries(stage s)
IBM->>PCH: read patch params (centroid_offset, geometry)
IBM->>IBM: if needed call s_compute_centroid_offset(ib_marker)
IBM->>MPI: reduce centroid data across ranks
IBM->>GRID: compute levelset / forces per cell (uses centroid-corrected coords)
GRID-->>IBM: cell-level forces/volumes
IBM->>TS: update IB velocities/angles/centroids (RK update)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.07% 44.16% +0.09%
==========================================
Files 71 71
Lines 20369 20417 +48
Branches 1986 1991 +5
==========================================
+ Hits 8978 9018 +40
- Misses 10246 10252 +6
- Partials 1145 1147 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
CodeAnt AI Incremental review completed. |
|
@sbryngelson I am getting desperate and tried passing the error to chatGPT, and it pointed out that the failure is a SIGTERM. The build is being terminated externally. It says the following: I confirmed this with some googling. Is it possible that we are running out of memory? It would explain why we keep failing at the same spot consistently and why master is the one failing. I would expect variable time if it were the walltime or workflow cancellation. Could be the problem. I will see if the other branch build successfully. |
|
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
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:1080">
P2: dynamic_viscosities entries are not initialized for fluids with Re(1)=0, but are used in the viscosity sum, leading to undefined values in force/torque calculations. Initialize the array or set zero in the Re==0 case before using it.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
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_ibm.fpp`:
- Around line 1076-1081: The spacing in the local dynamic viscosity accumulation
loop is inconsistent; re-run the code formatter or normalize spacing around
operators in the block that updates dynamic_viscosity inside the loop (variable
dynamic_viscosity, array q_prim_vf, field sf, and dynamic_viscosities used in
the do loop over fluid_idx). Ensure consistent spaces around assignment and
arithmetic operators (e.g., "dynamic_viscosity = dynamic_viscosity + ( ... )")
and align continuation/indentation to match the file's formatting conventions,
then save and re-run CI.
♻️ Duplicate comments (2)
src/simulation/m_ibm.fpp (2)
1056-1066: Pressure gradient indexing may be over-counting in multi-fluid cases.
This still usesq_prim_vf(E_idx + fluid_idx). IfE_idxis the single shared pressure (as in the usual MFC layout), thenE_idx + fluid_idxpoints to non-pressure fields and will double-count or miscompute forces. Please confirm.🔧 If pressure is shared, consider this fix
- local_force_contribution(:) = 0._wp - do fluid_idx = 0, num_fluids - 1 - local_force_contribution(1) = local_force_contribution(1) - (q_prim_vf(E_idx + fluid_idx)%sf(i + 1, j, k) - q_prim_vf(E_idx + fluid_idx)%sf(i - 1, j, k))/(2._wp*dx) - local_force_contribution(2) = local_force_contribution(2) - (q_prim_vf(E_idx + fluid_idx)%sf(i, j + 1, k) - q_prim_vf(E_idx + fluid_idx)%sf(i, j - 1, k))/(2._wp*dy) - cell_volume = abs(dx*dy) - if (num_dims == 3) then - dz = z_cc(k + 1) - z_cc(k) - local_force_contribution(3) = local_force_contribution(3) - (q_prim_vf(E_idx + fluid_idx)%sf(i, j, k + 1) - q_prim_vf(E_idx + fluid_idx)%sf(i, j, k - 1))/(2._wp*dz) - cell_volume = abs(cell_volume*dz) - end if - end do + local_force_contribution(:) = 0._wp + local_force_contribution(1) = local_force_contribution(1) - (q_prim_vf(E_idx)%sf(i + 1, j, k) - q_prim_vf(E_idx)%sf(i - 1, j, k))/(2._wp*dx) + local_force_contribution(2) = local_force_contribution(2) - (q_prim_vf(E_idx)%sf(i, j + 1, k) - q_prim_vf(E_idx)%sf(i, j - 1, k))/(2._wp*dy) + cell_volume = abs(dx*dy) + if (num_dims == 3) then + dz = z_cc(k + 1) - z_cc(k) + local_force_contribution(3) = local_force_contribution(3) - (q_prim_vf(E_idx)%sf(i, j, k + 1) - q_prim_vf(E_idx)%sf(i, j, k - 1))/(2._wp*dz) + cell_volume = abs(cell_volume*dz) + end if
1035-1039: Initializedynamic_viscositiesfor Re==0 fluids to avoid undefined viscous forces.
As written, entries stay uninitialized whenRe == 0, but they’re still used in the weighted sum, which can inject garbage into viscous forces/torques.🛠️ Proposed fix
- if (viscous) then - do fluid_idx = 1, num_fluids - if (fluid_pp(fluid_idx)%Re(1) /= 0._wp) dynamic_viscosities(fluid_idx) = 1._wp / fluid_pp(fluid_idx)%Re(1) - end do - end if + dynamic_viscosities = 0._wp + if (viscous) then + do fluid_idx = 1, num_fluids + if (fluid_pp(fluid_idx)%Re(1) /= 0._wp) then + dynamic_viscosities(fluid_idx) = 1._wp / fluid_pp(fluid_idx)%Re(1) + end if + end do + end if
|
Any chance I can get a branchmark run @sbryngelson or @wilfonba ? |
|
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
| do fluid_idx = 0, num_fluids - 1 | ||
| ! Get the pressure contribution to force via a finite difference to compute the 2D components of the gradient of the pressure and cell volume | ||
| local_force_contribution(1) = local_force_contribution(1) - (q_prim_vf(E_idx + fluid_idx)%sf(i + 1, j, k) - q_prim_vf(E_idx + fluid_idx)%sf(i - 1, j, k))/(2._wp*dx) ! force is the negative pressure gradient | ||
| local_force_contribution(2) = local_force_contribution(2) - (q_prim_vf(E_idx + fluid_idx)%sf(i, j + 1, k) - q_prim_vf(E_idx + fluid_idx)%sf(i, j - 1, k))/(2._wp*dy) | ||
| cell_volume = abs(dx*dy) | ||
| ! add the 3D component of the pressure gradient, if we are working in 3 dimensions | ||
| if (num_dims == 3) then | ||
| dz = z_cc(k + 1) - z_cc(k) | ||
| local_force_contribution(3) = local_force_contribution(3) - (q_prim_vf(E_idx + fluid_idx)%sf(i, j, k + 1) - q_prim_vf(E_idx + fluid_idx)%sf(i, j, k - 1))/(2._wp*dz) |
There was a problem hiding this comment.
Suggestion: Off-by-one indexing: the pressure-gradient loop uses do fluid_idx = 0, num_fluids - 1 but other code uses 1..num_fluids; this mixes 0-based and 1-based offsets when indexing q_prim_vf and is likely wrong or will index the wrong fields; change the loop to do fluid_idx = 1, num_fluids and index q_prim_vf(E_idx + fluid_idx - 1) so indexing is consistent with the rest of the code. [off-by-one]
Severity Level: Critical 🚨
- ❌ IB force computation for multi-fluid flows incorrect.
- ⚠️ Multi-fluid pressure-gradient selection inconsistent.
- ⚠️ Affects s_compute_ib_forces force aggregation path.| do fluid_idx = 0, num_fluids - 1 | |
| ! Get the pressure contribution to force via a finite difference to compute the 2D components of the gradient of the pressure and cell volume | |
| local_force_contribution(1) = local_force_contribution(1) - (q_prim_vf(E_idx + fluid_idx)%sf(i + 1, j, k) - q_prim_vf(E_idx + fluid_idx)%sf(i - 1, j, k))/(2._wp*dx) ! force is the negative pressure gradient | |
| local_force_contribution(2) = local_force_contribution(2) - (q_prim_vf(E_idx + fluid_idx)%sf(i, j + 1, k) - q_prim_vf(E_idx + fluid_idx)%sf(i, j - 1, k))/(2._wp*dy) | |
| cell_volume = abs(dx*dy) | |
| ! add the 3D component of the pressure gradient, if we are working in 3 dimensions | |
| if (num_dims == 3) then | |
| dz = z_cc(k + 1) - z_cc(k) | |
| local_force_contribution(3) = local_force_contribution(3) - (q_prim_vf(E_idx + fluid_idx)%sf(i, j, k + 1) - q_prim_vf(E_idx + fluid_idx)%sf(i, j, k - 1))/(2._wp*dz) | |
| do fluid_idx = 1, num_fluids | |
| ! Get the pressure contribution to force via a finite difference to compute the 2D components of the gradient of the pressure and cell volume | |
| local_force_contribution(1) = local_force_contribution(1) - (q_prim_vf(E_idx + fluid_idx - 1)%sf(i + 1, j, k) - q_prim_vf(E_idx + fluid_idx - 1)%sf(i - 1, j, k))/(2._wp*dx) ! force is the negative pressure gradient | |
| local_force_contribution(2) = local_force_contribution(2) - (q_prim_vf(E_idx + fluid_idx - 1)%sf(i, j + 1, k) - q_prim_vf(E_idx + fluid_idx - 1)%sf(i, j - 1, k))/(2._wp*dy) | |
| cell_volume = abs(dx*dy) | |
| ! add the 3D component of the pressure gradient, if we are working in 3 dimensions | |
| if (num_dims == 3) then | |
| dz = z_cc(k + 1) - z_cc(k) | |
| local_force_contribution(3) = local_force_contribution(3) - (q_prim_vf(E_idx + fluid_idx - 1)%sf(i, j, k + 1) - q_prim_vf(E_idx + fluid_idx - 1)%sf(i, j, k - 1))/(2._wp*dz) |
Steps of Reproduction ✅
1. Run a multi-fluid case (num_fluids >= 2) that exercises immersed boundary force
accumulation; s_compute_ib_forces is defined in src/simulation/m_ibm.fpp (subroutine
s_compute_ib_forces, start around line 1017).
2. When the triple loop over grid points hits an IB cell (ib_idx /= 0) the code executes
the pressure-gradient accumulation loop at src/simulation/m_ibm.fpp lines ~1060-1072 (the
shown local_force_contribution block).
3. The loop uses do fluid_idx = 0, num_fluids-1 and indexes q_prim_vf(E_idx + fluid_idx) —
the rest of the codebase consistently uses 1..num_fluids indexing for per-fluid fields
(see multiple occurrences of loops using 1..num_fluids, e.g., s_ibm_correct_state and
earlier sections).
4. On a multi-fluid run this off-by-one selection will pick the wrong q_prim_vf field for
at least one fluid component, producing incorrect pressure gradients and therefore
incorrect IB force contributions; this is observable in differing IB forces/torques vs.
expected values when comparing single- vs multi-fluid runs.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 1061:1069
**Comment:**
*Off By One: Off-by-one indexing: the pressure-gradient loop uses `do fluid_idx = 0, num_fluids - 1` but other code uses 1..num_fluids; this mixes 0-based and 1-based offsets when indexing `q_prim_vf` and is likely wrong or will index the wrong fields; change the loop to `do fluid_idx = 1, num_fluids` and index `q_prim_vf(E_idx + fluid_idx - 1)` so indexing is consistent with the rest of the code.
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. |
|
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. |
3 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
Add analytic moving IB velocities, centroid offset for airfoils, and multi-fluid IB force handling
What Changed
Impact
✅ Can run pitching or rotating airfoils about correct center✅ Clearer analytic IB motion setup in generated case files✅ Correct IB forces for multi-fluid viscous flows💡 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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.