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 field to track true center of mass for immersed boundaries; implements ellipse-based levelset computation; enables parametrized one-way MIBM motion via analytical expressions; fixes airfoil rotations to apply about true centroid instead of tip. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 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}
There was a problem hiding this comment.
High-level Suggestion
The toolchain code for analytic IB motion incorrectly allows spatial variables (x, y, z) in expressions for velocity and angular velocity. This should be refactored to only permit time (t) as a variable, as rigid body motion is not spatially dependent. [High-level, importance: 9]
Solution Walkthrough:
Before:
# toolchain/mfc/case.py
def __get_analytic_mib_fpp(self, print: bool) -> str:
# ...
def rhs_replace(match):
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())
# ...
for attribute, expr in items:
rhs = re.sub(r"[a-zA-Z]+", rhs_replace, expr)
lines.append(f" {attribute} = {rhs}")
# ...After:
# toolchain/mfc/case.py
def __get_analytic_mib_fpp(self, print: bool) -> str:
# ...
def rhs_replace(match):
return {
't': 'mytime',
'r': f'patch_ib({pid})%radius',
'e' : f'{math.e}',
}.get(match.group(), match.group())
# ...
for attribute, expr in items:
rhs = re.sub(r"[a-zA-Z]+", rhs_replace, expr)
lines.append(f" {attribute} = {rhs}")
# ...
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: In s_compute_centroid_offset, divide center_of_mass by the global cell count num_cells instead of the local count num_cells_local to correctly calculate the global center of mass after MPI reduction. [possible issue, importance: 9]
| center_of_mass = center_of_mass / real(num_cells_local, wp) | |
| center_of_mass = center_of_mass / real(num_cells, wp) |
| ! we will get NaNs in the levelset if we compute this outside the ellipse | ||
| 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 ! get the normal direction via the coordinate transofmration method | ||
| normal_vector = normal_vector / sqrt(dot_product(normal_vector, normal_vector)) ! normalize the vector | ||
| levelset_norm%sf(i, j, 0, ib_patch_id, :) = matmul(rotation, normal_vector) ! save after rotating the vector to the global frame | ||
|
|
||
| ! use the normal vector to set up the quadratic equation for the levelset, using A, B, and C in indices 1, 2, and 3 | ||
| quadratic_coeffs(1) = (normal_vector(1)/ellipse_coeffs(1))**2 + (normal_vector(2)/ellipse_coeffs(2))**2 | ||
| quadratic_coeffs(2) = 2._wp * ((xy_local(1)*normal_vector(1)/(ellipse_coeffs(1)**2)) + (xy_local(2)*normal_vector(2)/(ellipse_coeffs(2)**2))) | ||
| quadratic_coeffs(3) = (xy_local(1)/ellipse_coeffs(1))**2._wp + (xy_local(2)/ellipse_coeffs(2))**2._wp - 1._wp | ||
|
|
||
| ! compute the levelset with the quadratic equation [ -B + sqrt(B^2 - 4AC) ] / 2A | ||
| levelset%sf(i, j, 0, ib_patch_id) = -0.5_wp * (-quadratic_coeffs(2) + sqrt(quadratic_coeffs(2)**2._wp - 4._wp * quadratic_coeffs(1)*quadratic_coeffs(3))) / quadratic_coeffs(1) | ||
| end if |
There was a problem hiding this comment.
Suggestion: In s_ellipse_levelset, compute the level set for all points, not just those inside the ellipse, to ensure the signed distance function is correctly defined everywhere. Use an algebraic distance approximation for points outside the ellipse. [possible issue, importance: 8]
| ! we will get NaNs in the levelset if we compute this outside the ellipse | |
| 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 ! get the normal direction via the coordinate transofmration method | |
| normal_vector = normal_vector / sqrt(dot_product(normal_vector, normal_vector)) ! normalize the vector | |
| levelset_norm%sf(i, j, 0, ib_patch_id, :) = matmul(rotation, normal_vector) ! save after rotating the vector to the global frame | |
| ! use the normal vector to set up the quadratic equation for the levelset, using A, B, and C in indices 1, 2, and 3 | |
| quadratic_coeffs(1) = (normal_vector(1)/ellipse_coeffs(1))**2 + (normal_vector(2)/ellipse_coeffs(2))**2 | |
| quadratic_coeffs(2) = 2._wp * ((xy_local(1)*normal_vector(1)/(ellipse_coeffs(1)**2)) + (xy_local(2)*normal_vector(2)/(ellipse_coeffs(2)**2))) | |
| quadratic_coeffs(3) = (xy_local(1)/ellipse_coeffs(1))**2._wp + (xy_local(2)/ellipse_coeffs(2))**2._wp - 1._wp | |
| ! compute the levelset with the quadratic equation [ -B + sqrt(B^2 - 4AC) ] / 2A | |
| levelset%sf(i, j, 0, ib_patch_id) = -0.5_wp * (-quadratic_coeffs(2) + sqrt(quadratic_coeffs(2)**2._wp - 4._wp * quadratic_coeffs(1)*quadratic_coeffs(3))) / quadratic_coeffs(1) | |
| end if | |
| ! algebraic distance approximation | |
| 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) | |
| ! we still need to compute the normal vector for the ghost cell method | |
| 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 ! get the normal direction via the coordinate transofmration method | |
| normal_vector = normal_vector / sqrt(dot_product(normal_vector, normal_vector)) ! normalize the vector | |
| levelset_norm%sf(i, j, 0, ib_patch_id, :) = matmul(rotation, normal_vector) ! save after rotating the vector to the global frame | |
| end if |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
src/common/m_compute_levelset.fpp
Outdated
| quadratic_coeffs(3) = (xy_local(1)/ellipse_coeffs(1))**2._wp + (xy_local(2)/ellipse_coeffs(2))**2._wp - 1._wp | ||
|
|
||
| ! compute the levelset with the quadratic equation [ -B + sqrt(B^2 - 4AC) ] / 2A | ||
| levelset%sf(i, j, 0, ib_patch_id) = -0.5_wp * (-quadratic_coeffs(2) + sqrt(quadratic_coeffs(2)**2._wp - 4._wp * quadratic_coeffs(1)*quadratic_coeffs(3))) / quadratic_coeffs(1) |
There was a problem hiding this comment.
Suggestion: The quadratic root is computed with the wrong sign and denominator: the existing expression evaluates to (B - sqrt(...)) / (2A) instead of the correct (-B + sqrt(...)) / (2A), producing a levelset with the wrong sign; change the expression to use the standard quadratic formula denominator (2*A) with the correct numerator sign. [logic error]
Severity Level: Critical 🚨
- ❌ s_ellipse_levelset produces wrong ellipse geometry.
- ⚠️ Simulations using ellipse IB patches misrepresent surfaces.
- ⚠️ Postprocessing (levelset-dependent diagnostics) inaccurate.| levelset%sf(i, j, 0, ib_patch_id) = -0.5_wp * (-quadratic_coeffs(2) + sqrt(quadratic_coeffs(2)**2._wp - 4._wp * quadratic_coeffs(1)*quadratic_coeffs(3))) / quadratic_coeffs(1) | |
| levelset%sf(i, j, 0, ib_patch_id) = (-quadratic_coeffs(2) + sqrt(quadratic_coeffs(2)**2._wp - 4._wp * quadratic_coeffs(1)*quadratic_coeffs(3))) / (2._wp * quadratic_coeffs(1)) |
Steps of Reproduction ✅
1. Compile the PR code so module m_compute_levelset is built (file:
src/common/m_compute_levelset.fpp). This produces the subroutine s_ellipse_levelset
defined starting at line ~342.
2. Arrange to call s_ellipse_levelset(ib_patch_id, levelset, levelset_norm) directly (unit
test or existing levelset driver that dispatches ellipse patches). When a patch_ib entry
has non-zero length_x and length_y the routine runs the GPU loop (lines 371-393) and
evaluates the ellipse branch at lines ~377-391.
3. For any grid cell where the condition ((xy_local(1)/ellipse_coeffs(1))**2 +
(xy_local(2)/ellipse_coeffs(2))**2 <= 1._wp) is true (see lines ~377-388), the code
computes quadratic_coeffs and executes the assignment at line 390 (the existing_code
line). This is the concrete execution point for the reported issue.
4. Observe resulting levelset values: compare levelset%sf(i,j,0,ib_patch_id) produced at
line 390 against the analytic nearest-surface distance computed via (-B +
sqrt(B^2-4AC))/(2A). The current expression at line 390 simplifies to (B - sqrt(...))/(2A)
(wrong sign/ordering), producing an incorrect sign/branch for the root. Replacing the
expression with the improved code yields the expected analytic root.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/m_compute_levelset.fpp
**Line:** 390:390
**Comment:**
*Logic Error: The quadratic root is computed with the wrong sign and denominator: the existing expression evaluates to (B - sqrt(...)) / (2A) instead of the correct (-B + sqrt(...)) / (2A), producing a levelset with the wrong sign; change the expression to use the standard quadratic formula denominator (2*A) with the correct numerator sign.
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.| do i = 0, m | ||
| xy_local = [x_cc(i) - center(1), y_cc(j) - center(2), 0._wp] ! get coordinate frame centered on IB | ||
| xy_local = matmul(inverse_rotation, xy_local) ! rotate the frame into the IB's coordinates | ||
| xy_local = xy_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 centroid offset is subtracted from xy_local after rotating the global coordinate into the IB frame, but patch_ib(patch_id)%centroid_offset is likely defined in the global/model frame and must be rotated into the same frame before subtraction — otherwise the offset is applied in the wrong coordinate system causing misplaced airfoil geometry. Rotate the offset into the IB/local frame (or subtract the offset before rotation) so both vectors are in the same coordinate basis. [logic error]
Severity Level: Critical 🚨
- ❌ 2D airfoil placement in s_ib_airfoil mispositioned.
- ⚠️ Pitching-airfoil tests produce incorrect geometry mapping.| xy_local = xy_local - patch_ib(patch_id)%centroid_offset ! airfoils are a patch that require a centroid offset | |
| xy_local = xy_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 2D case that includes an airfoil patch (patch_ib(... )%geometry == 4). The 2D
branch in s_apply_ib_patches calls s_ib_airfoil (src/common/m_ib_patches.fpp — the 2D IB
patch logic that eventually invokes s_ib_airfoil).
2. Ensure the patch has a non-zero centroid_offset and a non-trivial rotation (set
patch_ib(patch_id)%centroid_offset and non-zero patch_ib(patch_id)%angles so
rotation_matrix_inverse != identity). Run to the phase where IB markers are populated so
s_ib_airfoil executes.
3. Inside s_ib_airfoil, execution computes xy_local and rotates it at the lines shown in
the diff (see src/common/m_ib_patches.fpp:279-281 where xy_local is computed and rotated,
then the subtraction at line 282). Because the code subtracts
patch_ib(patch_id)%centroid_offset without rotating it, the offset is applied in the
already-rotated/local frame incorrectly.
4. Observe incorrect IB marking: cell membership checks that follow (the if on xy_local(1)
at src/common/m_ib_patches.fpp:284 and subsequent assignments to ib_markers_sf) will mark
the airfoil in the wrong location or shape. This reproduces reliably when centroid_offset
is set and rotation is non-zero; the fix (rotate centroid_offset into local frame) aligns
coordinate bases and corrects marker placement.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/m_ib_patches.fpp
**Line:** 282:282
**Comment:**
*Logic Error: The centroid offset is subtracted from `xy_local` after rotating the global coordinate into the IB frame, but `patch_ib(patch_id)%centroid_offset` is likely defined in the global/model frame and must be rotated into the same frame before subtraction — otherwise the offset is applied in the wrong coordinate system causing misplaced airfoil geometry. Rotate the offset into the IB/local frame (or subtract the offset before rotation) so both vectors are in the same coordinate basis.
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.| 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.| patch_ib(i)%geometry == 12) then | ||
| call s_check_model_ib_patch_geometry(i) | ||
| else if (patch_ib(i)%geometry == 6) then | ||
| print *, "Ellipse Patch" |
There was a problem hiding this comment.
Suggestion: Missing validation hook: if ellipse geometry (6) is intended to be supported, the branch should invoke a dedicated validation routine (like other geometries do) rather than just printing; call the ellipse check subroutine so ellipse parameters are validated consistently with other patch types. [possible bug]
Severity Level: Critical 🚨
- ❌ Build fails at m_check_ib_patches.fpp:65 blocking compilation.
- ❌ CI and developer builds will be broken until function implemented.
- ⚠️ Prevents running any preprocessing or simulations.| print *, "Ellipse Patch" | |
| call s_check_ellipse_ib_patch_geometry(i) |
Steps of Reproduction ✅
1. Apply the proposed change that replaces the print with a call to
s_check_ellipse_ib_patch_geometry in src/pre_process/m_check_ib_patches.fpp
at lines 65-66 (i.e., insert the call in place of the print).
2. Run the project's build (compile all Fortran sources; e.g., invoke the project's
usual build target that compiles src/pre_process/m_check_ib_patches.fpp).
3. The Fortran compiler / linker will fail because there is no definition of
s_check_ellipse_ib_patch_geometry in the current codebase (a search of
src/pre_process/m_check_ib_patches.fpp and the file's subroutines shows no such
symbol). The error will reference the call site at
src/pre_process/m_check_ib_patches.fpp:65 (undefined reference / missing procedure).
4. Result: build fails immediately; unless s_check_ellipse_ib_patch_geometry is added
elsewhere, this change is a compile-time break, not a runtime validation improvement.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/pre_process/m_check_ib_patches.fpp
**Line:** 66:66
**Comment:**
*Possible Bug: Missing validation hook: if ellipse geometry (6) is intended to be supported, the branch should invoke a dedicated validation routine (like other geometries do) rather than just printing; call the ellipse check subroutine so ellipse parameters are validated consistently with other patch types.
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.| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL.analytic() | ||
| SIMULATION[f"patch_ib({ib_id})%angles({dir_id})"] = ParamType.REAL | ||
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL | ||
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL.analytic() |
There was a problem hiding this comment.
Suggestion: Type mismatch: assigning the result of ParamType.REAL.analytic() (a plain dict) directly into SIMULATION mixes raw dicts with ParamType Enum members. Later the code builds _properties = {k: v.value for k, v in ALL.items()} which assumes every value is an Enum with a .value attribute; a raw dict will raise AttributeError. Replace the two analytic assignments with the Enum member ParamType.REAL to keep dictionary values consistent and avoid the runtime AttributeError. [type error]
Severity Level: Critical 🚨
- ❌ Module import fails building schema and validator.
- ❌ get_validator() unusable; schema compilation blocked.
- ⚠️ Any tool consuming CASE options fails initialization.| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL.analytic() | |
| SIMULATION[f"patch_ib({ib_id})%angles({dir_id})"] = ParamType.REAL | |
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL | |
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL.analytic() | |
| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL | |
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL |
Steps of Reproduction ✅
1. Open the module file toolchain/mfc/run/case_dicts.py and locate the
ParamType.analytic() implementation (function at toolchain/mfc/run/case_dicts.py:17-22).
The analytic() returns a plain dict for ParamType.REAL.
2. Observe the SIMULATION assignments inside the ib loop at the hunk lines where the PR
added analytic variants (see toolchain/mfc/run/case_dicts.py lines 369 and 371): those two
lines assign the raw dicts returned from ParamType.REAL.analytic() into SIMULATION.
3. Import the module (e.g., python -c "import toolchain.mfc.run.case_dicts as c") or call
get_validator() from toolchain/mfc/run/case_dicts.py. Module import triggers building of
ALL and then the comprehension _properties = { k: v.value for k, v in ALL.items() } (the
_properties line is defined immediately after ALL is assembled in the same file).
4. At that comprehension, Python evaluates v.value for every value in ALL. For the two
entries assigned above, v is a plain dict and has no .value attribute; this raises
AttributeError: 'dict' object has no attribute 'value'. The import or get_validator() call
fails with that exception.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/mfc/run/case_dicts.py
**Line:** 369:371
**Comment:**
*Type Error: Type mismatch: assigning the result of `ParamType.REAL.analytic()` (a plain dict) directly into `SIMULATION` mixes raw dicts with `ParamType` Enum members. Later the code builds `_properties = {k: v.value for k, v in ALL.items()}` which assumes every value is an Enum with a `.value` attribute; a raw dict will raise AttributeError. Replace the two analytic assignments with the Enum member `ParamType.REAL` to keep dictionary values consistent and avoid the runtime AttributeError.
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 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.
| # Patch: Cylinder Immersed Boundary | ||
| "patch_ib(1)%geometry": 6, | ||
| "patch_ib(1)%x_centroid": 1.5e-03, | ||
| "patch_ib(1)%y_centroid": 3.0e-03, | ||
| "patch_ib(1)%length_x": 0.4e-03, | ||
| "patch_ib(1)%length_y": 0.2e-03, | ||
| "patch_ib(1)%slip": "F", |
There was a problem hiding this comment.
Update the comment to reflect the actual geometry.
The comment on line 84 says "Cylinder Immersed Boundary" but this patch defines an ellipse (geometry=6). Update the comment to match the actual geometry being configured.
- # Patch: Cylinder Immersed Boundary
+ # Patch: Ellipse Immersed Boundary
"patch_ib(1)%geometry": 6,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Patch: Cylinder Immersed Boundary | |
| "patch_ib(1)%geometry": 6, | |
| "patch_ib(1)%x_centroid": 1.5e-03, | |
| "patch_ib(1)%y_centroid": 3.0e-03, | |
| "patch_ib(1)%length_x": 0.4e-03, | |
| "patch_ib(1)%length_y": 0.2e-03, | |
| "patch_ib(1)%slip": "F", | |
| # Patch: Ellipse Immersed Boundary | |
| "patch_ib(1)%geometry": 6, | |
| "patch_ib(1)%x_centroid": 1.5e-03, | |
| "patch_ib(1)%y_centroid": 3.0e-03, | |
| "patch_ib(1)%length_x": 0.4e-03, | |
| "patch_ib(1)%length_y": 0.2e-03, | |
| "patch_ib(1)%slip": "F", |
🤖 Prompt for AI Agents
In `@examples/2D_ibm_ellipse/case.py` around lines 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.
![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.
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.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/simulation/m_ibm.fpp, line 1121:
<comment>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.</comment>
<file context>
@@ -1080,6 +1081,60 @@ contains
+ 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)
+
+ ! assign the centroid offset as a vector pointing from the true COM to the "centroid" in the input file and replace the current centroid
</file context>
| center_of_mass = center_of_mass / real(num_cells_local, wp) | |
| if (num_cells > 0) then | |
| center_of_mass = center_of_mass / real(num_cells, wp) | |
| else | |
| center_of_mass = 0._wp | |
| end if |
| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL.analytic() | ||
| SIMULATION[f"patch_ib({ib_id})%angles({dir_id})"] = ParamType.REAL | ||
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL | ||
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL.analytic() |
There was a problem hiding this comment.
P1: Same issue as with vel: assigning ParamType.REAL.analytic() (a plain dict) will cause AttributeError when building _properties. Use ParamType.REAL instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At toolchain/mfc/run/case_dicts.py, line 371:
<comment>Same issue as with `vel`: assigning `ParamType.REAL.analytic()` (a plain dict) will cause `AttributeError` when building `_properties`. Use `ParamType.REAL` instead.</comment>
<file context>
@@ -368,9 +366,9 @@ def analytic(self):
+ SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL.analytic()
SIMULATION[f"patch_ib({ib_id})%angles({dir_id})"] = ParamType.REAL
- SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL
+ SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL.analytic()
for cmp_id, cmp in enumerate(["x", "y", "z"]):
</file context>
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL.analytic() | |
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL |
|
|
||
| for dir_id in range(1, 4): | ||
| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL | ||
| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL.analytic() |
There was a problem hiding this comment.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At toolchain/mfc/run/case_dicts.py, line 369:
<comment>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.</comment>
<file context>
@@ -368,9 +366,9 @@ def analytic(self):
for dir_id in range(1, 4):
- SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL
+ SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL.analytic()
SIMULATION[f"patch_ib({ib_id})%angles({dir_id})"] = ParamType.REAL
- SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL
</file context>
| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL.analytic() | |
| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL |
| xy_local = matmul(inverse_rotation, xy_local) | ||
|
|
||
| ! we will get NaNs in the levelset if we compute this outside the ellipse | ||
| if ((xy_local(1)/ellipse_coeffs(1))**2 + (xy_local(2)/ellipse_coeffs(2))**2 <= 1._wp) then |
There was a problem hiding this comment.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/common/m_compute_levelset.fpp, line 377:
<comment>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.</comment>
<file context>
@@ -336,6 +339,62 @@ contains
+ xy_local = matmul(inverse_rotation, xy_local)
+
+ ! we will get NaNs in the levelset if we compute this outside the ellipse
+ if ((xy_local(1)/ellipse_coeffs(1))**2 + (xy_local(2)/ellipse_coeffs(2))**2 <= 1._wp) then
+
+ normal_vector = xy_local
</file context>
|
|
||
| # function that defines how we will replace variable names with | ||
| # values from the case file | ||
| def rhs_replace(match): |
There was a problem hiding this comment.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At toolchain/mfc/case.py, line 227:
<comment>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.</comment>
<file context>
@@ -181,6 +196,72 @@ def rhs_replace(match):
+
+ # function that defines how we will replace variable names with
+ # values from the case file
+ def rhs_replace(match):
+ return {
+ 'x': 'x_cc(i)', 'y': 'y_cc(j)', 'z': 'z_cc(k)',
</file context>
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
| xy_local = matmul(inverse_rotation, xy_local) ! rotate the frame into the IB's coordinate | ||
| xy_local = xy_local - patch_ib(ib_patch_id)%centroid_offset ! airfoils are a patch that require a centroid offset |
There was a problem hiding this comment.
Suggestion: Coordinate-frame mixup: centroid_offset is subtracted after applying inverse_rotation, which treats the offset as if it were expressed in the rotated/local frame; if centroid_offset is defined in the global frame (as center is), this produces incorrect local coordinates and wrong nearest-point logic. Subtract the centroid_offset before rotation so the subtraction happens in the same (global) coordinate frame as the initial subtraction from center. [logic error]
Severity Level: Critical 🚨
- ❌ Airfoil 2D levelset geometry computed incorrectly.
- ⚠️ Nearest-point selection for airfoil grids unreliable.
- ⚠️ Normal vectors for IB forcing may be incorrect.
- ⚠️ Affects pitching airfoil test cases using centroid_offset.| xy_local = matmul(inverse_rotation, xy_local) ! rotate the frame into the IB's coordinate | |
| xy_local = xy_local - patch_ib(ib_patch_id)%centroid_offset ! airfoils are a patch that require a centroid offset | |
| xy_local = xy_local - patch_ib(ib_patch_id)%centroid_offset ! airfoils are a patch that require a centroid offset | |
| xy_local = matmul(inverse_rotation, xy_local) ! rotate the frame into the IB's coordinate |
Steps of Reproduction ✅
1. Build and run the pitching airfoil test described in the PR description (the provided
pitching-airfoil configuration used to validate the change). The levelset computation for
the airfoil is invoked from module m_compute_levelset, subroutine s_airfoil_levelset in
src/common/m_compute_levelset.fpp.
2. Execution enters s_airfoil_levelset and iterates the 2D grid (do i = 0, m; do j = 0,
n). See code computing local coordinates at src/common/m_compute_levelset.fpp lines 95-97
where xy_local is set, rotated, then centroid_offset is subtracted.
3. If the airfoil patch has a non-zero patch_ib(ib_patch_id)%centroid_offset (set up by
the IB initialization for that patch), the subtraction at line 97 occurs after the
inverse_rotation at line 96, i.e., in the already-rotated/local frame. If centroid_offset
is provided in the global/frame-consistent coordinates (the usual convention for patch_ib
fields), this produces an incorrect xy_local.
4. Observe incorrect nearest-point selection and normals: levelset%sf and levelset_norm%sf
entries computed in s_airfoil_levelset (later in the same loop) will be shifted or have
incorrect normals. Reproduce by comparing levelset output or visualized geometry between
runs with centroid_offset==0 and centroid_offset!=0; the incorrect subtraction order
produces a displaced/rotated airfoil geometry in the test case.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/m_compute_levelset.fpp
**Line:** 96:97
**Comment:**
*Logic Error: Coordinate-frame mixup: `centroid_offset` is subtracted after applying `inverse_rotation`, which treats the offset as if it were expressed in the rotated/local frame; if `centroid_offset` is defined in the global frame (as `center` is), this produces incorrect local coordinates and wrong nearest-point logic. Subtract the `centroid_offset` before rotation so the subtraction happens in the same (global) coordinate frame as the initial subtraction from `center`.
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.| moving_immersed_boundary_flag = .true. | ||
| end if | ||
| call s_update_ib_rotation_matrix(i) | ||
| call s_compute_centroid_offset(i) |
There was a problem hiding this comment.
Suggestion: Performance / correctness: s_compute_centroid_offset is called for every IB inside the initial setup loop before IB markers are populated; this causes an expensive full-domain scan per IB and also runs before ib_markers%sf is initialized/populated (leading to zero-cell counts). Remove the call from this early loop and compute centroid offsets later after IB patches/markers are applied and buffers populated. [performance]
Severity Level: Critical 🚨
- ❌ Startup initialization spends excessive CPU time.
- ⚠️ Centroid offsets computed from empty ib_markers.
- ❌ Ghost-point velocity uses wrong centroids.
- ⚠️ Pitching airfoil initial motion affected.| call s_compute_centroid_offset(i) |
Steps of Reproduction ✅
1. Start the code path that calls the IBM setup routine: the public subroutine s_ibm_setup
in src/simulation/m_ibm.fpp is executed (see s_ibm_setup loop at lines 98-108).
2. Inside s_ibm_setup (lines 98-108) the code iterates do i = 1, num_ibs and
unconditionally calls s_compute_centroid_offset(i) at line 106 before any IB marker
population steps.
3. s_compute_centroid_offset (defined at lines 1155-1207) scans ib_markers%sf over the
full domain to count cells (it depends on ib_markers contents). At this point ib_markers
was only allocated earlier (s_initialize_ibm_module) but not populated by
s_apply_ib_patches / s_populate_ib_buffers yet, so entries are effectively empty/zero.
4. Observable effects when reproducing: you will see s_compute_centroid_offset perform a
full-domain nested loop (i=0..m, j=0..n, k=0..p) for every IB (costly) and compute zero
cell counts / zero centroid results because ib_markers%sf contains no IB assignments yet.
This reproduces wasted work and incorrect centroid results on startup.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 106:106
**Comment:**
*Performance: Performance / correctness: s_compute_centroid_offset is called for every IB inside the initial setup loop before IB markers are populated; this causes an expensive full-domain scan per IB and also runs before `ib_markers%sf` is initialized/populated (leading to zero-cell counts). Remove the call from this early loop and compute centroid offsets later after IB patches/markers are applied and buffers populated.
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.| 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, wp) | ||
There was a problem hiding this comment.
Suggestion: Division-by-zero and incorrect averaging: the code reduces the summed coordinates and the integer cell count and unconditionally divides by num_cells. If no cells are found for the IB across all ranks this will divide by zero. Reduce the total cell-volume (or count) and check the global total before dividing; if zero, set a safe default and return. [possible bug]
Severity Level: Critical 🚨
- ❌ Simulation startup can crash (division by zero).
- ❌ Pitching airfoil initialisation fails.
- ⚠️ Any IB with geometry types 4,5,11,12 affected.
- ⚠️ MPI job may produce NaNs across ranks.| ! also reduce the accumulated cell-volume across ranks and guard against zero total volume | |
| call s_mpi_allreduce_sum(total_volume_local, total_volume) | |
| if (total_volume <= 0._wp) then | |
| ! No volume found for this IB across all ranks: set zero offset and exit safely | |
| patch_ib(ib_marker)%centroid_offset(:) = [0._wp, 0._wp, 0._wp] | |
| return | |
| end if | |
| center_of_mass = center_of_mass/total_volume |
Steps of Reproduction ✅
1. Trigger the startup path that runs s_ibm_setup (src/simulation/m_ibm.fpp lines 98-108)
so that s_compute_centroid_offset is invoked (calls at line 106).
2. Inside s_compute_centroid_offset (subroutine at lines 1155-1207) the routine tallies
num_cells_local on each rank and then calls s_mpi_allreduce_integer_sum to produce
num_cells (see lines 1188-1192).
3. If no grid cell was assigned to this IB on any rank (num_cells == 0 after allreduce)
the code executes center_of_mass = center_of_mass/real(num_cells, wp) at line 1192,
producing a division-by-zero (or invalid NaN) at runtime.
4. Reproduce by running a minimal case where the IB geometry guarded by the if-condition
(geometry == 4,5,11,12) is present but ib_markers are empty (this occurs during current
setup ordering), observe crash/NaN at startup originating from s_compute_centroid_offset
lines 1188-1192.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 1193:1193
**Comment:**
*Possible Bug: Division-by-zero and incorrect averaging: the code reduces the summed coordinates and the integer cell count and unconditionally divides by `num_cells`. If no cells are found for the IB across all ranks this will divide by zero. Reduce the total cell-volume (or count) and check the global total before dividing; if zero, set a safe default and return.
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. |
1 participant
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
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
Analytic moving immersed-boundary velocities and correct airfoil centroid handling
What Changed
Impact
✅ Accurate pitching/rotating airfoils about their true center✅ Can run prescribed analytic IB motions (e.g., oscillating/prescribed trajectories)✅ Fewer setup mismatches for STL/airfoil geometries💡 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
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.