Open
Conversation
…o levelset-refactor
…o levelset-refactor
Member
Author
|
Would I be able to get approval for benchmark on this? |
sbryngelson
approved these changes
Feb 12, 2026
Member
Author
|
I don't see the benchmarks running and I also don't see it listed as a check to perform anymore. Was there something that modifies when benchmark is run in the toolchain rework? |
sbryngelson
approved these changes
Feb 12, 2026
sbryngelson
approved these changes
Feb 12, 2026
Member
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Approve to trigger benchmark CI
sbryngelson
approved these changes
Feb 12, 2026
Member
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Approve to trigger benchmark CI
sbryngelson
previously approved these changes
Feb 12, 2026
Member
|
understood, i see the issue and will fix. in the meantime can you look and comment on the remaining more targeted AI reviewer comments? |
Member
Author
|
Yeah, I saw you working on it. Didn't meant to ping you if you saw it immediately. I will do that until the runners are resolved. |
Contributor
|
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:XXL
Contributor
|
CodeAnt AI Incremental review completed. |
Member
|
i tuned the reviewers a lot. i'm glad to see it actually did something useful. runners should work now modulo their regular fickleness, though i did add something to make them a bit less fickle. |
Contributor
|
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:XXL
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1048
to
+1066
| subroutine s_write_serial_ib_data(time_step) | ||
|
|
||
| integer, intent(in) :: time_step | ||
| character(LEN=path_len + 2*name_len) :: file_path | ||
| character(LEN=path_len + 2*name_len) :: t_step_dir | ||
|
|
||
| ! Creating or overwriting the time-step root directory | ||
| write (t_step_dir, '(A,I0,A,I0)') trim(case_dir)//'/p_all' | ||
| write (t_step_dir, '(a,i0,a,i0)') trim(case_dir)//'/p_all/p', & | ||
| proc_rank, '/', time_step | ||
| write (file_path, '(A,I0,A)') trim(t_step_dir)//'/ib.dat' | ||
|
|
||
| open (2, FILE=trim(file_path), & | ||
| FORM='unformatted', & | ||
| STATUS='new') | ||
|
|
||
| write (2) ib_markers%sf(0:m, 0:n, 0:p); close (2) | ||
|
|
||
| end subroutine |
Contributor
There was a problem hiding this comment.
Suggestion: Remove the redundant and malformed write statement in s_write_serial_ib_data to prevent a runtime formatting error. [possible issue, importance: 7]
Suggested change
| subroutine s_write_serial_ib_data(time_step) | |
| integer, intent(in) :: time_step | |
| character(LEN=path_len + 2*name_len) :: file_path | |
| character(LEN=path_len + 2*name_len) :: t_step_dir | |
| ! Creating or overwriting the time-step root directory | |
| write (t_step_dir, '(A,I0,A,I0)') trim(case_dir)//'/p_all' | |
| write (t_step_dir, '(a,i0,a,i0)') trim(case_dir)//'/p_all/p', & | |
| proc_rank, '/', time_step | |
| write (file_path, '(A,I0,A)') trim(t_step_dir)//'/ib.dat' | |
| open (2, FILE=trim(file_path), & | |
| FORM='unformatted', & | |
| STATUS='new') | |
| write (2) ib_markers%sf(0:m, 0:n, 0:p); close (2) | |
| end subroutine | |
| subroutine s_write_serial_ib_data(time_step) | |
| integer, intent(in) :: time_step | |
| character(LEN=path_len + 2*name_len) :: file_path | |
| character(LEN=path_len + 2*name_len) :: t_step_dir | |
| ! Creating or overwriting the time-step root directory | |
| write (t_step_dir, '(a,i0,a,i0)') trim(case_dir)//'/p_all/p', & | |
| proc_rank, '/', time_step | |
| write (file_path, '(A,I0,A)') trim(t_step_dir)//'/ib.dat' | |
| open (2, FILE=trim(file_path), & | |
| FORM='unformatted', & | |
| STATUS='new') | |
| write (2) ib_markers%sf(0:m, 0:n, 0:p); close (2) | |
| end subroutine |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| ! compute the levelset with the quadratic equation [ -B + sqrt(B^2 - 4AC) ] / 2A | ||
| gp%levelset = -0.5_wp*(-quadratic_coeffs(2) + sqrt(quadratic_coeffs(2)**2._wp - 4._wp*quadratic_coeffs(1)*quadratic_coeffs(3)))/quadratic_coeffs(1) | ||
|
|
Contributor
There was a problem hiding this comment.
Suggestion: Guard the quadratic discriminant in the ellipse levelset computation against small negative values due to roundoff to prevent NaNs from the square root. [custom_rule]
Severity Level: Minor
Suggested change
| gp%levelset = -0.5_wp*(-quadratic_coeffs(2) + sqrt(max(quadratic_coeffs(2)**2._wp - 4._wp*quadratic_coeffs(1)*quadratic_coeffs(3), 0._wp)))/quadratic_coeffs(1) |
Why it matters? ⭐
The discriminant can be slightly negative due to roundoff even when mathematically non-negative, causing sqrt() to produce NaN. Wrapping the discriminant with max(..., 0._wp) prevents taking sqrt of a tiny negative value and resolves a numerical stability issue (precision/roundoff) highlighted by the project's review priorities.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_compute_levelset.fpp
**Line:** 434:434
**Comment:**
*Custom Rule: Guard the quadratic discriminant in the ellipse levelset computation against small negative values due to roundoff to prevent NaNs from the square root.
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.| boundary(2) = 0.5_wp*length(3) | ||
| dist_sides_vec = (/0, 0, 1/) | ||
| dist_surface_vec = (/1, 1, 0/) | ||
| end if |
Contributor
There was a problem hiding this comment.
Suggestion: Add a fallback branch in the cylinder handling to return a safe default when all cylinder lengths are effectively zero, avoiding use of uninitialized direction vectors. [custom_rule]
Severity Level: Minor
Suggested change
| end if | |
| else | |
| gp%levelset = 0._wp | |
| gp%levelset_norm = 0._wp | |
| return |
Why it matters? ⭐
The existing code leaves dist_sides_vec/dist_surface_vec uninitialized when all length components are (near) zero, which can lead to undefined behavior later in the routine. Adding an explicit else branch that sets a safe default levelset and normal and returns prevents use of uninitialized direction vectors and fixes a correctness/memory safety issue. This directly addresses the PR review priorities for correctness and memory (avoid using uninitialized data) from the MFC review rules.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_compute_levelset.fpp
**Line:** 618:618
**Comment:**
*Custom Rule: Add a fallback branch in the cylinder handling to return a safe default when all cylinder lengths are effectively zero, avoiding use of uninitialized direction vectors.
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.
Comment on lines
+359
to
+361
| min_dist = side_dists(1) | ||
| idx = 1 | ||
|
|
Contributor
There was a problem hiding this comment.
Suggestion: In the rectangle levelset computation, the distances to the right and top sides have reversed sign compared to the cuboid and sphere definitions, so points both inside and outside on those sides can yield the same (negative) levelset sign, breaking inside/outside classification; redefine those distances so that the sign convention is consistent with other geometries. [logic error]
Severity Level: Critical 🚨
- ⚠️ Inconsistent levelset sign only for rectangular immersed boundaries.
- ⚠️ Downstream code assuming sign uniformity misclassifies rectangle regions.
- ⚠️ Rectangular IB test cases may show asymmetric behavior.
Suggested change
| min_dist = side_dists(1) | |
| idx = 1 | |
| side_dists(2) = xy_local(1) - top_right(1) | |
| side_dists(3) = bottom_left(2) - xy_local(2) | |
| side_dists(4) = xy_local(2) - top_right(2) |
Steps of Reproduction ✅
1. Configure a 2D simulation that uses a rectangular immersed boundary (geometry id 3 in
`patch_ib(:)%geometry`), so that `s_apply_levelset` in
`src/simulation/m_compute_levelset.fpp:28-97` is called with ghost points whose
`gps(i)%ib_patch_id` refers to that rectangle.
2. For a ghost point inside the rectangle but closer to the right side than the left
(e.g., `xy_local(1) > 0`), call `s_rectangle_levelset` directly (lines 315-385) or via
`s_apply_levelset`'s 2D branch (lines 61-77) so execution reaches the block at lines
358-367.
3. At lines 358-361, observe that `side_dists(2) = top_right(1) - xy_local(1)` is positive
for interior points near the right side, while `side_dists(1) = bottom_left(1) -
xy_local(1)` is negative for comparable interior points near the left side; `gp%levelset`
is assigned `side_dists(idx)` at line 369.
4. Compare with `s_cuboid_levelset` in the same file (lines 437-535), where distances are
defined as `Left - x` and `x - Right`, giving `gp%levelset < 0` inside and `> 0` outside
for all faces; in contrast, for rectangles the current formulas yield interior points with
different signs depending on whether they are near left or right/top sides, so sign cannot
consistently encode inside vs outside.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_compute_levelset.fpp
**Line:** 359:361
**Comment:**
*Logic Error: In the rectangle levelset computation, the distances to the right and top sides have reversed sign compared to the cuboid and sphere definitions, so points both inside and outside on those sides can yield the same (negative) levelset sign, breaking inside/outside classification; redefine those distances so that the sign convention is consistent with other geometries.
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.
Comment on lines
+395
to
+433
|
|
||
| real(wp) :: length_x, length_y | ||
| real(wp), dimension(1:3) :: xy_local, normal_vector !< x and y coordinates in local IB frame | ||
| real(wp), dimension(2) :: center !< x and y coordinates in local IB frame | ||
| real(wp), dimension(1:3, 1:3) :: rotation, inverse_rotation | ||
|
|
||
| integer :: i, j, k !< Loop index variables | ||
| integer :: idx !< Shortest path direction indicator | ||
| integer :: ib_patch_id !< patch ID | ||
|
|
||
| ib_patch_id = gp%ib_patch_id | ||
| i = gp%loc(1) | ||
| j = gp%loc(2) | ||
|
|
||
| length_x = patch_ib(ib_patch_id)%length_x | ||
| length_y = patch_ib(ib_patch_id)%length_y | ||
| center(1) = patch_ib(ib_patch_id)%x_centroid | ||
| center(2) = patch_ib(ib_patch_id)%y_centroid | ||
| inverse_rotation(:, :) = patch_ib(ib_patch_id)%rotation_matrix_inverse(:, :) | ||
| rotation(:, :) = patch_ib(ib_patch_id)%rotation_matrix(:, :) | ||
|
|
||
| ellipse_coeffs(1) = 0.5_wp*length_x | ||
| ellipse_coeffs(2) = 0.5_wp*length_y | ||
|
|
||
| xy_local = [x_cc(i) - center(1), y_cc(j) - center(2), 0._wp] | ||
| xy_local = matmul(inverse_rotation, xy_local) | ||
|
|
||
| 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 transformation method | ||
| normal_vector = normal_vector/sqrt(dot_product(normal_vector, normal_vector)) ! normalize the vector | ||
| gp%levelset_norm = 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 | ||
| gp%levelset = -0.5_wp*(-quadratic_coeffs(2) + sqrt(quadratic_coeffs(2)**2._wp - 4._wp*quadratic_coeffs(1)*quadratic_coeffs(3)))/quadratic_coeffs(1) |
Contributor
There was a problem hiding this comment.
Suggestion: In the ellipse levelset, the quadratic discriminant is used directly inside a sqrt without guarding against small negative values caused by floating-point roundoff, which can produce NaNs and crash or corrupt the levelset when a point lies close to the ellipse boundary; clamping the discriminant to be non-negative before sqrt avoids this. [possible bug]
Severity Level: Critical 🚨
- ❌ NaN levelset values possible for ellipse ghost points near boundary.
- ⚠️ Can destabilize or crash runs with elliptical boundaries.
- ⚠️ Hard-to-reproduce failures at specific grid/geometry alignments.
Suggested change
| real(wp) :: length_x, length_y | |
| real(wp), dimension(1:3) :: xy_local, normal_vector !< x and y coordinates in local IB frame | |
| real(wp), dimension(2) :: center !< x and y coordinates in local IB frame | |
| real(wp), dimension(1:3, 1:3) :: rotation, inverse_rotation | |
| integer :: i, j, k !< Loop index variables | |
| integer :: idx !< Shortest path direction indicator | |
| integer :: ib_patch_id !< patch ID | |
| ib_patch_id = gp%ib_patch_id | |
| i = gp%loc(1) | |
| j = gp%loc(2) | |
| length_x = patch_ib(ib_patch_id)%length_x | |
| length_y = patch_ib(ib_patch_id)%length_y | |
| center(1) = patch_ib(ib_patch_id)%x_centroid | |
| center(2) = patch_ib(ib_patch_id)%y_centroid | |
| inverse_rotation(:, :) = patch_ib(ib_patch_id)%rotation_matrix_inverse(:, :) | |
| rotation(:, :) = patch_ib(ib_patch_id)%rotation_matrix(:, :) | |
| ellipse_coeffs(1) = 0.5_wp*length_x | |
| ellipse_coeffs(2) = 0.5_wp*length_y | |
| xy_local = [x_cc(i) - center(1), y_cc(j) - center(2), 0._wp] | |
| xy_local = matmul(inverse_rotation, xy_local) | |
| 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 transformation method | |
| normal_vector = normal_vector/sqrt(dot_product(normal_vector, normal_vector)) ! normalize the vector | |
| gp%levelset_norm = 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 | |
| gp%levelset = -0.5_wp*(-quadratic_coeffs(2) + sqrt(quadratic_coeffs(2)**2._wp - 4._wp*quadratic_coeffs(1)*quadratic_coeffs(3)))/quadratic_coeffs(1) | |
| real(wp) :: disc | |
| real(wp) :: length_x, length_y | |
| real(wp), dimension(1:3) :: xy_local, normal_vector !< x and y coordinates in local IB frame | |
| real(wp), dimension(2) :: center !< x and y coordinates in local IB frame | |
| real(wp), dimension(1:3, 1:3) :: rotation, inverse_rotation | |
| integer :: i, j, k !< Loop index variables | |
| integer :: idx !< Shortest path direction indicator | |
| integer :: ib_patch_id !< patch ID | |
| ib_patch_id = gp%ib_patch_id | |
| i = gp%loc(1) | |
| j = gp%loc(2) | |
| length_x = patch_ib(ib_patch_id)%length_x | |
| length_y = patch_ib(ib_patch_id)%length_y | |
| center(1) = patch_ib(ib_patch_id)%x_centroid | |
| center(2) = patch_ib(ib_patch_id)%y_centroid | |
| inverse_rotation(:, :) = patch_ib(ib_patch_id)%rotation_matrix_inverse(:, :) | |
| rotation(:, :) = patch_ib(ib_patch_id)%rotation_matrix(:, :) | |
| ellipse_coeffs(1) = 0.5_wp*length_x | |
| ellipse_coeffs(2) = 0.5_wp*length_y | |
| xy_local = [x_cc(i) - center(1), y_cc(j) - center(2), 0._wp] | |
| xy_local = matmul(inverse_rotation, xy_local) | |
| 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 transformation method | |
| normal_vector = normal_vector/sqrt(dot_product(normal_vector, normal_vector)) ! normalize the vector | |
| gp%levelset_norm = 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 | |
| disc = quadratic_coeffs(2)**2._wp - 4._wp*quadratic_coeffs(1)*quadratic_coeffs(3) | |
| disc = max(disc, 0._wp) | |
| ! compute the levelset with the quadratic equation [ -B + sqrt(B^2 - 4AC) ] / 2A | |
| gp%levelset = -0.5_wp*(-quadratic_coeffs(2) + sqrt(disc))/quadratic_coeffs(1) |
Steps of Reproduction ✅
1. Configure a 2D simulation using an elliptical immersed boundary (geometry id 6) so that
`s_apply_levelset` in `src/simulation/m_compute_levelset.fpp:28-97` enters the 2D branch
(lines 61-77) and calls `s_ellipse_levelset` for ghost points on that patch.
2. Select a ghost point whose local coordinates place it numerically very close to the
ellipse boundary (i.e., `xy_local` at lines 419-420 such that the exact mathematical
discriminant `B^2 - 4AC` for the quadratic constructed at lines 428-430 is zero).
3. Due to floating-point roundoff in the computations of `quadratic_coeffs(1:3)` (lines
428-430), the actual value of `quadratic_coeffs(2)**2._wp -
4._wp*quadratic_coeffs(1)*quadratic_coeffs(3)` at line 433 can become a small negative
number (e.g. `-1e-15_wp`) instead of exactly zero.
4. When `sqrt` is evaluated on this negative discriminant at line 433, the Fortran runtime
produces a NaN for the square root, which propagates into `gp%levelset` and then into any
later use of that ghost point's levelset (e.g., interpolation or forcing), potentially
destabilizing the simulation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_compute_levelset.fpp
**Line:** 395:433
**Comment:**
*Possible Bug: In the ellipse levelset, the quadratic discriminant is used directly inside a sqrt without guarding against small negative values caused by floating-point roundoff, which can produce NaNs and crash or corrupt the levelset when a point lies close to the ellipse boundary; clamping the discriminant to be non-negative before sqrt avoids this.
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.|
|
||
| if (dist_side < abs(dist_surface)) then | ||
| ! if the closest edge is flat | ||
| gp%levelset = -dist_side |
Contributor
There was a problem hiding this comment.
Suggestion: For the cylinder, when the closest feature is a flat end cap, the levelset is always set to the negative of the axial distance, so points outside the cylinder along its axis incorrectly get negative levelset values instead of positive, which breaks the expected signed-distance behavior and inside/outside logic; the sign should depend on whether the point is between the end caps or beyond them. [logic error]
Severity Level: Critical 🚨
- ⚠️ Mis-signed levelset near cylindrical end caps along axis.
- ⚠️ Outside axial points classified as inside in cylinder cases.
- ⚠️ Can distort immersed-boundary forcing near cylinder ends.
Suggested change
| gp%levelset = -dist_side | |
| if (side_pos >= boundary(1) .and. side_pos <= boundary(2)) then | |
| ! point lies between the end caps (inside along the axis) | |
| gp%levelset = -dist_side | |
| else | |
| ! point lies outside the end caps along the axis | |
| gp%levelset = dist_side | |
| end if |
Steps of Reproduction ✅
1. Configure a 3D simulation with a cylindrical immersed boundary (geometry id 10) so that
`s_apply_levelset` in `src/simulation/m_compute_levelset.fpp:28-97` is called with ghost
points belonging to that patch (branch at lines 48-52 calls `s_cylinder_levelset`).
2. Choose a ghost point located along the cylinder axis but just beyond one end cap in
local coordinates (e.g., `xyz_local` such that `side_pos > boundary(2)` at lines 604-617
and radial position within the cylinder radius so `dist_surface` is strongly negative).
3. When `s_cylinder_levelset` executes for this point (lines 571-646), `dist_side` at
lines 624-627 will be small and positive, `abs(dist_surface)` at lines 628-629 will be
larger, so the `if (dist_side < abs(dist_surface))` condition at line 631 is true and the
flat-cap branch is taken.
4. At line 633, `gp%levelset` is set to `-dist_side`, a negative value, even though the
point lies outside the cylinder volume along its axis, whereas other geometry routines in
this module (e.g., `s_sphere_levelset` at lines 537-569 and `s_cuboid_levelset` at lines
437-535) return negative values inside and positive outside; this makes axial exterior
points indistinguishable from interior points based on sign.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_compute_levelset.fpp
**Line:** 633:633
**Comment:**
*Logic Error: For the cylinder, when the closest feature is a flat end cap, the levelset is always set to the negative of the axial distance, so points outside the cylinder along its axis incorrectly get negative levelset values instead of positive, which breaks the expected signed-distance behavior and inside/outside logic; the sign should depend on whether the point is between the end caps or beyond them.
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.
Contributor
|
CodeAnt AI Incremental review completed. |
Member
|
the merge conflicts should be trivial |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Description
This PR is a significant refactor of the levelset code. It contains
Fixes #1011
How Has This Been Tested?
This currently passes all tests on MacOS and Ubuntu operating systms using the GNU compiler.
PR Type
Enhancement, Refactor
Description
Removed levelset computation from preprocessing: IB markers and levelset values are no longer computed during pre-processing; all computations now occur during simulation runtime
Eliminated global levelset arrays: Replaced global
levelsetandlevelset_normarrays with local storage in theghost_pointderived type, reducing memory allocation by orders of magnitudeParallelized levelset computation over ghost points: Refactored levelset code to parallelize over ghost points instead of grid cells, improving memory efficiency and GPU performance
GPU-accelerated STL model processing: Implemented GPU compute for STL IB markers and levelset values by loading STL models into global memory with separate parallelized loops
New
m_compute_levelsetmodule: Created dedicated module for all levelset computations with geometry-specific functions (circles, rectangles, ellipses, spheres, cylinders, airfoils, STL models)Added STL model configuration parameters: Extended IB patch parameters to support
model_filepath,model_spc,model_threshold, and transformation parameters (model_translate,model_rotate)Simplified data I/O: Removed levelset and levelset norm from preprocessing output; added dedicated IB data output subroutines for simulation runtime
Updated test infrastructure: Adjusted grid resolution for circle test cases and updated golden test files to reflect removal of pre-computed IB markers
Diagram Walkthrough
File Walkthrough
8 files
m_ib_patches.fpp
Refactor levelset computation from preprocessing to simulation runtimesrc/simulation/m_ib_patches.fpp
m_compute_levelsetmodule dependency and levelset computationcalls from
s_apply_ib_patchess_instantiate_STL_models()subroutine to load and preprocessSTL models during initialization
s_ib_model()to use pre-instantiated models from globalmodelsarray instead of computing levelsets inlineetavariable declarations and replaced with local scopeusage in geometry-specific functions
offsetvariable to airfoil subroutines for cleaner centroidoffset handling
m_data_output.fpp
Remove levelset data output from preprocessingsrc/pre_process/m_data_output.fpp
ib_markers,levelset, andlevelset_normparameters from dataoutput subroutine signatures
levelset norm data
s_write_abstract_data_files()m_ibm.fpp
Refactor IBM module to use ghost-point-based levelsetssrc/simulation/m_ibm.fpp
levelsetandlevelset_normarrays and replaced withghost-point-local storage
s_instantiate_STL_models()during IBM setups_apply_ib_patches()calls to remove levelset parameterss_compute_image_points()to use levelset values fromghost_pointstructure instead of global arrayss_update_mib()to call news_apply_levelset()instead ofcomputing levelsets inline
patch_id_fparray allocationm_start_up.fpp
Remove levelset restart file I/O from simulation startupsrc/simulation/m_start_up.fpp
from restart files
s_write_ib_data_file()after IBM setup duringinitialization
s_read_ic_data_files()call signature by removing IB markerparameter
m_start_up.fpp
Remove levelset handling from preprocessing startupsrc/pre_process/m_start_up.fpp
m_ib_patchesmodule importib_markers,levelset, andlevelset_normvariable declarationsand allocations
s_read_ic_data_files()interface by removing IB markerparameter
m_mpi_common.fpp
Simplify MPI data initialization for IBMsrc/common/m_mpi_common.fpp
levelsetandlevelset_normparameters froms_initialize_mpi_data()subroutinestructures
m_initial_condition.fpp
Remove IBM-related variables from preprocessingsrc/pre_process/m_initial_condition.fpp
m_ib_patchesmodule importib_markers,levelset, andlevelset_normvariable declarationss_apply_ib_patches()call from preprocessingm_time_steppers.fpp
Remove levelset parameters from immersed boundary updatesrc/simulation/m_time_steppers.fpp
s_update_mibsubroutine call by removinglevelsetandlevelset_normparameterssimulation rather than passed as arguments
5 files
m_compute_levelset.fpp
New module for ghost-point-based levelset computationsrc/simulation/m_compute_levelset.fpp
boundaries
s_apply_levelset()subroutine that computes levelsets forghost points instead of grid cells
ellipses, spheres, cylinders, airfoils, and STL models
ghost_pointderived type instead of global arrays
efficiency
m_data_output.fpp
Add dedicated IB data output subroutinessrc/simulation/m_data_output.fpp
s_write_serial_ib_data()ands_write_parallel_ib_data()for IB data outputs_write_ib_data_file()wrapper to handle both serial andparallel I/O
routines
parameters
m_global_parameters.fpp
Add default initialization for IB patch parameterssrc/simulation/m_global_parameters.fpp
patch_ib()array with default values forall IB patch parameters
translate, rotate)
m_derived_types.fpp
Add derived types for model storage and ghost point levelsetssrc/common/m_derived_types.fpp
t_model_arrayderived type to store STL model data withboundary vertices and interpolation information
ghost_pointderived type withlevelsetandlevelset_normfields for local storage
point structure
case_dicts.py
Add STL model and transformation parameters to IB configurationtoolchain/mfc/run/case_dicts.py
model_filepath,model_spc, andmodel_thresholdmodel_translateandmodel_rotatefor each spatial directionmodel_scaleparameter for potential future use7 files
cases.py
Adjust grid resolution for circle test casestoolchain/mfc/test/cases.py
n=49grid resolution for improvedprecision
circular geometries
golden.txt
Remove IB markers from golden test outputtests/7FA04E95/golden.txt
ib_markersoutput line from golden test dataand stored globally
golden.txt
Remove IB markers output from test golden filetests/5600D63B/golden.txt
D/ib_markers.00.datwith all zerovalues
D/cons.5.00.000050.datline with updated valuesgolden-metadata.txt
Update test metadata with new build environment detailstests/7F70E665/golden-metadata.txt
OpenMP : OFFconfiguration linegolden-metadata.txt
Update test metadata with new build environment detailstests/F60D6594/golden-metadata.txt
OpenMP : OFFconfiguration linegolden-metadata.txt
Update test metadata with new build environment detailstests/4F5A5E32/golden-metadata.txt
OpenMP : OFFconfiguration linegolden-metadata.txt
Update test metadata with new build environment detailstests/8D8F6424/golden-metadata.txt
OpenMP : OFFconfiguration line3 files
golden-metadata.txt
Update golden test metadata and build environmenttests/B0CE19C5/golden-metadata.txt
hash
paths, system information)
golden-metadata.txt
Update golden test metadata and build environmenttests/7DCE34B4/golden-metadata.txt
hash
paths, system information)
golden-metadata.txt
Update golden test metadata and build environmenttests/6171E9D4/golden-metadata.txt
hash
paths, system information)
specifications
63 files
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
CodeAnt-AI Description
Compute immersed-boundary levelsets at runtime and remove levelset file I/O
What Changed
Impact
✅ Smaller memory footprint for levelset data✅ No restart dependency on precomputed levelset files✅ STL models available at runtime for accurate ghost-point geometry checks💡 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.