-
Notifications
You must be signed in to change notification settings - Fork 124
Invicid two-way fluid-structure interaction #1075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 17 commits
5c3b79a
1f39342
9b687b8
22dba76
42512af
85ea1b1
b38796e
0cbe4b9
9ea3020
3f68040
676aa73
8094216
5d2fc37
b7f00d1
f25e61d
c82a623
25ee314
7c70fd4
0c221ae
ec0ccf6
a4967c9
8531946
cfe6714
a216ed4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,105 @@ | ||||||||||
| import json | ||||||||||
| import math | ||||||||||
|
|
||||||||||
| Mu = 1.84e-05 | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Unused variable: Severity Level: Critical 🚨
Suggested change
Why it matters? ⭐The variable Mu is defined but not referenced anywhere in the file after the PR. It's dead code and should be removed to avoid confusion; this is a harmless cleanup that doesn't change behavior. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** examples/2D_mibm_cylinder_in_cross_flow/case.py
**Line:** 4:4
**Comment:**
*Possible Bug: Unused variable: `Mu` is assigned but never referenced; this is dead code and can be removed to avoid misleading readers about intended viscous computations.
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. |
||||||||||
| gam_a = 1.4 | ||||||||||
|
|
||||||||||
| # Configuring case dictionary | ||||||||||
| print( | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Module-level side effect: the code prints the JSON at import time which causes unexpected output whenever this module is imported; wrap the printing in an Severity Level: Minor
Suggested change
Why it matters? ⭐The module currently performs a print at import time which is a side effect and can surprise callers who import this module. Wrapping the print in if name == "main": is the conventional, low-risk fix to ensure the JSON is only emitted when the script is executed directly. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** examples/2D_mibm_cylinder_in_cross_flow/case.py
**Line:** 8:8
**Comment:**
*Logic Error: Module-level side effect: the code prints the JSON at import time which causes unexpected output whenever this module is imported; wrap the printing in an `if __name__ == "__main__":` guard so the JSON is only printed when the script is executed directly.
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. |
||||||||||
| json.dumps( | ||||||||||
| { | ||||||||||
| # Logistics | ||||||||||
| "run_time_info": "T", | ||||||||||
| # Computational Domain Parameters | ||||||||||
| # For these computations, the cylinder is placed at the (0,0,0) | ||||||||||
| # domain origin. | ||||||||||
| # axial direction | ||||||||||
| "x_domain%beg": 0.0e00, | ||||||||||
| "x_domain%end": 6.0e-03, | ||||||||||
| # r direction | ||||||||||
| "y_domain%beg": 0.0e00, | ||||||||||
| "y_domain%end": 6.0e-03, | ||||||||||
| "cyl_coord": "F", | ||||||||||
| "m": 200, | ||||||||||
| "n": 200, | ||||||||||
| "p": 0, | ||||||||||
| "dt": 6.0e-6, | ||||||||||
| "t_step_start": 0, | ||||||||||
| "t_step_stop": 10000, # 10000, | ||||||||||
| "t_step_save": 100, | ||||||||||
| # Simulation Algorithm Parameters | ||||||||||
| # Only one patches are necessary, the air tube | ||||||||||
| "num_patches": 1, | ||||||||||
| # Use the 5 equation model | ||||||||||
| "model_eqns": 2, | ||||||||||
| "alt_soundspeed": "F", | ||||||||||
| # One fluids: air | ||||||||||
| "num_fluids": 1, | ||||||||||
| # time step | ||||||||||
| "mpp_lim": "F", | ||||||||||
| # Correct errors when computing speed of sound | ||||||||||
| "mixture_err": "T", | ||||||||||
| # Use TVD RK3 for time marching | ||||||||||
| "time_stepper": 3, | ||||||||||
| # Use WENO5 | ||||||||||
| "weno_order": 5, | ||||||||||
| "weno_eps": 1.0e-16, | ||||||||||
| "weno_Re_flux": "T", | ||||||||||
| "weno_avg": "T", | ||||||||||
| "avg_state": 2, | ||||||||||
| "mapped_weno": "T", | ||||||||||
| "null_weights": "F", | ||||||||||
| "mp_weno": "T", | ||||||||||
| "riemann_solver": 2, | ||||||||||
| "wave_speeds": 1, | ||||||||||
| # We use ghost-cell | ||||||||||
| "bc_x%beg": -3, | ||||||||||
| "bc_x%end": -3, | ||||||||||
| "bc_y%beg": -3, | ||||||||||
| "bc_y%end": -3, | ||||||||||
| # Set IB to True and add 1 patch | ||||||||||
| "ib": "T", | ||||||||||
| "num_ibs": 1, | ||||||||||
| "viscous": "T", | ||||||||||
| # Formatted Database Files Structure Parameters | ||||||||||
| "format": 1, | ||||||||||
| "precision": 2, | ||||||||||
| "prim_vars_wrt": "T", | ||||||||||
| "E_wrt": "T", | ||||||||||
| "parallel_io": "T", | ||||||||||
| # Patch: Constant Tube filled with air | ||||||||||
| # Specify the cylindrical air tube grid geometry | ||||||||||
| "patch_icpp(1)%geometry": 3, | ||||||||||
| "patch_icpp(1)%x_centroid": 3.0e-03, | ||||||||||
| # Uniform medium density, centroid is at the center of the domain | ||||||||||
| "patch_icpp(1)%y_centroid": 3.0e-03, | ||||||||||
| "patch_icpp(1)%length_x": 6.0e-03, | ||||||||||
| "patch_icpp(1)%length_y": 6.0e-03, | ||||||||||
| # Specify the patch primitive variables | ||||||||||
| "patch_icpp(1)%vel(1)": 0.05e00, | ||||||||||
| "patch_icpp(1)%vel(2)": 0.0e00, | ||||||||||
| "patch_icpp(1)%pres": 1.0e00, | ||||||||||
| "patch_icpp(1)%alpha_rho(1)": 1.0e00, | ||||||||||
| "patch_icpp(1)%alpha(1)": 1.0e00, | ||||||||||
| # Patch: Cylinder Immersed Boundary | ||||||||||
| "patch_ib(1)%geometry": 2, | ||||||||||
| "patch_ib(1)%x_centroid": 1.5e-03, | ||||||||||
| "patch_ib(1)%y_centroid": 4.5e-03, | ||||||||||
| "patch_ib(1)%radius": 0.3e-03, | ||||||||||
| "patch_ib(1)%slip": "F", | ||||||||||
| "patch_ib(1)%moving_ibm": 2, | ||||||||||
| "patch_ib(1)%vel(2)": -0.1, | ||||||||||
| "patch_ib(1)%angles(1)": 0.0, # x-axis rotation in radians | ||||||||||
| "patch_ib(1)%angles(2)": 0.0, # y-axis rotation | ||||||||||
| "patch_ib(1)%angles(3)": 0.0, # z-axis rotation | ||||||||||
| "patch_ib(1)%angular_vel(1)": 0.0, # x-axis rotational velocity in radians per second | ||||||||||
| "patch_ib(1)%angular_vel(2)": 0.0, # y-axis rotation | ||||||||||
| "patch_ib(1)%angular_vel(3)": 0.0, # z-axis rotation | ||||||||||
| "patch_ib(1)%mass": 0.0001, # z-axis rotation | ||||||||||
|
||||||||||
| "patch_ib(1)%mass": 0.0001, # z-axis rotation | |
| "patch_ib(1)%mass": 0.0001, # mass of the immersed boundary object |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Incorrect comment: The comment # z-axis rotation is wrong for the mass parameter. This appears to be a copy-paste error from the angular velocity comments above.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/2D_mibm_cylinder_in_cross_flow/case.py, line 98:
<comment>Incorrect comment: The comment `# z-axis rotation` is wrong for the `mass` parameter. This appears to be a copy-paste error from the angular velocity comments above.</comment>
<file context>
@@ -0,0 +1,105 @@
+ "patch_ib(1)%angular_vel(1)": 0.0, # x-axis rotational velocity in radians per second
+ "patch_ib(1)%angular_vel(2)": 0.0, # y-axis rotation
+ "patch_ib(1)%angular_vel(3)": 0.0, # z-axis rotation
+ "patch_ib(1)%mass": 0.0001, # z-axis rotation
+ # Fluids Physical Parameters
+ "fluid_pp(1)%gamma": 1.0e00 / (gam_a - 1.0e00), # 2.50(Not 1.40)
</file context>
| "patch_ib(1)%mass": 0.0001, # z-axis rotation | |
| "patch_ib(1)%mass": 0.0001, # mass of the immersed boundary |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -332,6 +332,8 @@ module m_derived_types | |||||
|
|
||||||
| !! Patch conditions for moving imersed boundaries | ||||||
| integer :: moving_ibm ! 0 for no moving, 1 for moving, 2 for moving on forced path | ||||||
| real(wp) :: mass, moment ! mass and moment of inertia of object used to compute forces in 2-way coupling | ||||||
| real(wp), dimension(1:3) :: force, torque ! vectors for the computed force and torque values applied to an IB | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The new Severity Level: Minor
Suggested change
Why it matters? ⭐Changing dimension(1:3) to dimension(3) makes the declaration consistent with other 3-component vectors in the module and avoids accidental reliance on a non-unit lower bound. It's a cosmetic/clarity improvement with no behavioral impact. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/common/m_derived_types.fpp
**Line:** 336:336
**Comment:**
*Logic Error: The new `force` and `torque` arrays use an explicit lower bound notation `dimension(1:3)` while many other 3-component vectors in this file use `dimension(3)` or `L(3)/R(3)` notation; this inconsistency can confuse callers and increase the risk of indexing mistakes. Use the canonical `dimension(3)` form for 3-element vectors to match the rest of the module and avoid subtle off-by-bound assumptions.
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. |
||||||
| real(wp), dimension(1:3) :: vel | ||||||
| real(wp), dimension(1:3) :: step_vel ! velocity array used to store intermediate steps in the time_stepper module | ||||||
| real(wp), dimension(1:3) :: angular_vel | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -472,6 +472,30 @@ contains | |||||||||
| end subroutine s_mpi_allreduce_sum | ||||||||||
| !> This subroutine follows the behavior of the s_mpi_allreduce_sum subroutine | ||||||||||
| !> with the additional feature that it reduces an array of vectors. | ||||||||||
| impure subroutine s_mpi_allreduce_vectors_sum(var_loc, var_glb, num_vectors, vector_length) | ||||||||||
| integer, intent(in) :: num_vectors, vector_length | ||||||||||
| real(wp), dimension(:, :), intent(in) :: var_loc | ||||||||||
| real(wp), dimension(:, :), intent(out) :: var_glb | ||||||||||
|
Comment on lines
+479
to
+481
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Intent mismatch for in-place reduction: Severity Level: Minor
Suggested change
Why it matters? ⭐This is a valid correctness issue. INTENT(OUT) permits the compiler to treat the actual argument as undefined on entry (and even reallocate/erase it), which makes using MPI_IN_PLACE unsafe when the caller passes the same buffer for send and receive. Changing var_glb to INTENT(INOUT) accurately documents and guarantees the value is preserved on entry and is the correct contract for potential in-place MPI calls. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/common/m_mpi_common.fpp
**Line:** 479:481
**Comment:**
*Logic Error: Intent mismatch for in-place reduction: `var_glb` is declared with INTENT(OUT) but the subroutine uses MPI_IN_PLACE when the caller passes the same buffer for send and receive; INTENT(OUT) allows the compiler to undefine or reallocate the actual argument before the call which makes in-place MPI operations unsafe. Change `var_glb` to INTENT(INOUT) so the caller's data is preserved on entry for MPI_IN_PLACE use.
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. |
||||||||||
| #ifdef MFC_MPI | ||||||||||
| integer :: ierr !< Generic flag used to identify and report MPI errors | ||||||||||
| ! Performing the reduction procedure | ||||||||||
| if (loc(var_loc) == loc(var_glb)) then | ||||||||||
| call MPI_Allreduce(MPI_IN_PLACE, var_glb, num_vectors*vector_length, & | ||||||||||
| mpi_p, MPI_SUM, MPI_COMM_WORLD, ierr) | ||||||||||
| else | ||||||||||
| call MPI_Allreduce(var_loc, var_glb, num_vectors*vector_length, & | ||||||||||
| mpi_p, MPI_SUM, MPI_COMM_WORLD, ierr) | ||||||||||
| end if | ||||||||||
| #endif | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Missing non-MPI fallback: when Prompt for AI agents
Suggested change
✅ Addressed in |
||||||||||
| end subroutine s_mpi_allreduce_vectors_sum | ||||||||||
| !> The following subroutine takes the input local variable | ||||||||||
| !! from all processors and reduces to the sum of all | ||||||||||
| !! values. The reduced variable is recorded back onto the | ||||||||||
|
|
||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High-level SuggestionThe force and torque calculations in Solution Walkthrough:Before:subroutine s_compute_ib_forces(pressure)
...
do i = 0, m
do j = 0, n
do k = 0, p
...
! Incorrect pressure gradient using cell-center coordinates in denominator
pressure_divergence(1) = (pressure(i+1,j,k) - pressure(i-1,j,k)) / (2._wp * x_cc(i))
pressure_divergence(2) = (pressure(i,j+1,k) - pressure(i,j-1,k)) / (2._wp * y_cc(j))
! Incorrect cell volume calculation using coordinates
cell_volume = x_cc(i) * y_cc(j)
if (num_dims == 3) then
pressure_divergence(3) = (pressure(i,j,k+1) - pressure(i,j,k-1)) / (2._wp * z_cc(k))
cell_volume = cell_volume * z_cc(k)
end if
forces(ib_idx, :) = forces(ib_idx, :) - (pressure_divergence * cell_volume)
...
end do
end do
end do
end subroutine
After:subroutine s_compute_ib_forces(pressure)
...
! Grid spacings (dx, dy, dz) should be available or computed
! e.g., dx = x_cc(1) - x_cc(0) for a uniform grid
do i = 0, m
do j = 0, n
do k = 0, p
...
! Correct pressure gradient using grid spacing
pressure_divergence(1) = (pressure(i+1,j,k) - pressure(i-1,j,k)) / (2._wp * dx)
pressure_divergence(2) = (pressure(i,j+1,k) - pressure(i,j-1,k)) / (2._wp * dy)
! Correct cell volume calculation
cell_volume = dx * dy
if (num_dims == 3) then
pressure_divergence(3) = (pressure(i,j,k+1) - pressure(i,j,k-1)) / (2._wp * dz)
cell_volume = cell_volume * dz
end if
forces(ib_idx, :) = forces(ib_idx, :) - (pressure_divergence * cell_volume)
...
end do
end do
end do
end subroutine
|
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -101,7 +101,6 @@ contains | |||||||||||||||||||||||||||||||||||||
| end if | ||||||||||||||||||||||||||||||||||||||
| call s_update_ib_rotation_matrix(i) | ||||||||||||||||||||||||||||||||||||||
| end do | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| $:GPU_ENTER_DATA(copyin='[patch_ib]') | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ! Allocating the patch identities bookkeeping variable | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -197,6 +196,21 @@ contains | |||||||||||||||||||||||||||||||||||||
| type(ghost_point) :: gp | ||||||||||||||||||||||||||||||||||||||
| type(ghost_point) :: innerp | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ! set the Moving IBM interior Pressure Values | ||||||||||||||||||||||||||||||||||||||
| do patch_id = 1, num_ibs | ||||||||||||||||||||||||||||||||||||||
| if (patch_ib(patch_id)%moving_ibm == 2) then | ||||||||||||||||||||||||||||||||||||||
| do j = 0, m | ||||||||||||||||||||||||||||||||||||||
| do k = 0, n | ||||||||||||||||||||||||||||||||||||||
| do l = 0, p | ||||||||||||||||||||||||||||||||||||||
| if (ib_markers%sf(j,k,l) == patch_id) then | ||||||||||||||||||||||||||||||||||||||
| q_prim_vf(E_idx)%sf(j, k, l) = 1._wp | ||||||||||||||||||||||||||||||||||||||
| end if | ||||||||||||||||||||||||||||||||||||||
| end do | ||||||||||||||||||||||||||||||||||||||
| end do | ||||||||||||||||||||||||||||||||||||||
| end do | ||||||||||||||||||||||||||||||||||||||
| end if | ||||||||||||||||||||||||||||||||||||||
| end do | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (num_gps > 0) then | ||||||||||||||||||||||||||||||||||||||
| $:GPU_PARALLEL_LOOP(private='[i,physical_loc,dyn_pres,alpha_rho_IP, alpha_IP,pres_IP,vel_IP,vel_g,vel_norm_IP,r_IP, v_IP,pb_IP,mv_IP,nmom_IP,presb_IP,massv_IP,rho, gamma,pi_inf,Re_K,G_K,Gs,gp,innerp,norm,buf, radial_vector, rotation_velocity, j,k,l,q,qv_K,c_IP,nbub,patch_id]') | ||||||||||||||||||||||||||||||||||||||
| do i = 1, num_gps | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -244,6 +258,17 @@ contains | |||||||||||||||||||||||||||||||||||||
| if (surface_tension) then | ||||||||||||||||||||||||||||||||||||||
| q_prim_vf(c_idx)%sf(j, k, l) = c_IP | ||||||||||||||||||||||||||||||||||||||
| end if | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ! set the pressure | ||||||||||||||||||||||||||||||||||||||
| do q = 1, num_fluids | ||||||||||||||||||||||||||||||||||||||
| if (patch_ib(patch_id)%moving_ibm == 0) then | ||||||||||||||||||||||||||||||||||||||
| q_prim_vf(E_idx)%sf(j, k, l) = pres_IP | ||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||
| ! TODO :: improve for two fluid | ||||||||||||||||||||||||||||||||||||||
| q_prim_vf(E_idx)%sf(j, k, l) = pres_IP / (1._wp - 2._wp*abs(levelset%sf(j,k,l,patch_id)*alpha_rho_IP(q)/pres_IP) * dot_product(patch_ib(patch_id)%force/patch_ib(patch_id)%mass, levelset_norm%sf(j,k,l,patch_id,:))) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| q_prim_vf(E_idx)%sf(j, k, l) = pres_IP / (1._wp - 2._wp*abs(levelset%sf(j,k,l,patch_id)*alpha_rho_IP(q)/pres_IP) * dot_product(patch_ib(patch_id)%force/patch_ib(patch_id)%mass, levelset_norm%sf(j,k,l,patch_id,:))) | |
| if (pres_IP == 0._wp) then | |
| ! protect against division by zero: fall back to the interpolated pressure | |
| q_prim_vf(E_idx)%sf(j, k, l) = pres_IP | |
| else | |
| buf = 1._wp - 2._wp*abs(levelset%sf(j,k,l,patch_id)*alpha_rho_IP(q)/pres_IP) * dot_product(patch_ib(patch_id)%force/patch_ib(patch_id)%mass, levelset_norm%sf(j,k,l,patch_id,:)) | |
| if (abs(buf) < 1.e-30_wp) then | |
| ! denominator too small, avoid unstable division | |
| q_prim_vf(E_idx)%sf(j, k, l) = pres_IP | |
| else | |
| q_prim_vf(E_idx)%sf(j, k, l) = pres_IP / buf | |
| end if | |
| end if |
Why it matters? ⭐
The current expression divides by pres_IP and a denominator that can be zero or very small, which can produce NaNs or runtime errors. Guarding the division (check pres_IP and the computed denominator) is a legitimate safety fix. The proposed improved code prevents unstable divisions and falls back to a safe value.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 268:268
**Comment:**
*Possible Bug: Division-by-zero risk when computing modified pressure for moving immersed boundaries: the formula divides by `pres_IP` (and later by the computed denominator) with no guard, so if `pres_IP == 0` (or the denominator becomes 0) this will raise a runtime error or produce NaNs; guard the division and handle the zero/near-zero case before dividing.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Pressure gradient calculation uses cell center coordinate x_cc(i) instead of grid spacing. Central difference formula should divide by 2*dx (grid spacing), not the coordinate value x_cc(i).
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 1025:
<comment>Pressure gradient calculation uses cell center coordinate `x_cc(i)` instead of grid spacing. Central difference formula should divide by `2*dx` (grid spacing), not the coordinate value `x_cc(i)`.</comment>
<file context>
@@ -969,6 +994,67 @@ contains
+ else
+ radial_vector = [x_cc(i), y_cc(j), 0._wp] - [patch_ib(ib_idx)%x_centroid, patch_ib(ib_idx)%y_centroid, 0._wp] ! get the vector pointing to the grid cell
+ end if
+ pressure_divergence(1) = (pressure(i+1, j, k) - pressure(i-1, j, k)) / (2._wp * x_cc(i))
+ pressure_divergence(2) = (pressure(i, j+1, k) - pressure(i, j-1, k)) / (2._wp * y_cc(j))
+ cell_volume = x_cc(i) * y_cc(j)
</file context>
✅ Addressed in cfe6714
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Suggestion: Correct the finite difference calculation for pressure_divergence and cell_volume. The denominators for the gradient should use grid spacing differences, and the volume should be calculated from grid spacings, not cell-center coordinates. [possible issue, importance: 9]
| pressure_divergence(1) = (pressure(i+1, j, k) - pressure(i-1, j, k)) / (2._wp * x_cc(i)) | |
| pressure_divergence(2) = (pressure(i, j+1, k) - pressure(i, j-1, k)) / (2._wp * y_cc(j)) | |
| cell_volume = x_cc(i) * y_cc(j) | |
| if (num_dims == 3) then | |
| pressure_divergence(3) = (pressure(i, j, k+1) - pressure(i, j, k-1)) / (2._wp * z_cc(k)) | |
| cell_volume = cell_volume * z_cc(k) | |
| else | |
| pressure_divergence(3) = 0._wp | |
| end if | |
| pressure_divergence(1) = (pressure(i+1, j, k) - pressure(i-1, j, k)) / (x_cc(i+1) - x_cc(i-1)) | |
| pressure_divergence(2) = (pressure(i, j+1, k) - pressure(i, j-1, k)) / (y_cc(j+1) - y_cc(j-1)) | |
| cell_volume = (x_cc(i) - x_cc(i-1)) * (y_cc(j) - y_cc(j-1)) | |
| if (num_dims == 3) then | |
| pressure_divergence(3) = (pressure(i, j, k+1) - pressure(i, j, k-1)) / (z_cc(k+1) - z_cc(k-1)) | |
| cell_volume = cell_volume * (z_cc(k) - z_cc(k-1)) | |
| else | |
| pressure_divergence(3) = 0._wp | |
| end if |
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Debug print statement should be removed from production code.
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 1054:
<comment>Debug print statement should be removed from production code.</comment>
<file context>
@@ -969,6 +994,67 @@ contains
+ patch_ib(i)%torque(:) = matmul(patch_ib(i)%rotation_matrix_inverse, torques(i, :)) ! torques must be computed in the local coordinates of the IB
+ end do
+
+ print *, patch_ib(1)%force(1:2)
+
+ end subroutine s_compute_ib_forces
</file context>
✅ Addressed in 0c221ae
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Debug print statement should be removed from production code.
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 1054:
<comment>Debug print statement should be removed from production code.</comment>
<file context>
@@ -969,6 +994,67 @@ contains
+ patch_ib(i)%torque(:) = matmul(patch_ib(i)%rotation_matrix_inverse, torques(i, :)) ! torques must be computed in the local coordinates of the IB
+ end do
+
+ print *, patch_ib(1)%force(1:2)
+
+ end subroutine s_compute_ib_forces
</file context>
✅ Addressed in 0c221ae
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Correct the normalization of the axis vector by using the sum of squares of its components (sum(axis**2)) instead of the sum of the components (sum(axis)) to calculate its magnitude. [possible issue, importance: 9]
| if (p == 0) then | |
| axis = [0, 1, 0] | |
| else if (sqrt(sum(axis**2)) == 0) then | |
| ! if the object is not actually rotating at this time, return a dummy value and exit | |
| patch_ib(ib_marker)%moment = 1._wp | |
| return | |
| else | |
| axis = axis / sqrt(sum(axis)) | |
| end if | |
| if (p == 0) then | |
| axis = [0, 1, 0] | |
| else if (sqrt(sum(axis**2)) == 0) then | |
| ! if the object is not actually rotating at this time, return a dummy value and exit | |
| patch_ib(ib_marker)%moment = 1._wp | |
| return | |
| else | |
| axis = axis / sqrt(sum(axis**2)) | |
| end if |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Suggestion: Wrong variable used when computing analytic moment for rectangular geometry: the expression uses patch_ib(i)%mass but i is undefined here (should use the subroutine argument ib_marker); this will read a wrong/undefined mass value and produce incorrect result. [logic error]
Severity Level: Minor
| patch_ib(ib_marker)%moment = patch_ib(i)%mass * (patch_ib(ib_marker)%length_x**2 + patch_ib(ib_marker)%length_y**2) / 6._wp | |
| patch_ib(ib_marker)%moment = patch_ib(ib_marker)%mass * (patch_ib(ib_marker)%length_x**2 + patch_ib(ib_marker)%length_y**2) / 6._wp |
Why it matters? ⭐
The expression reads patch_ib(i)%mass but there is no loop variable i in this branch — the subroutine parameter is ib_marker. Using i is incorrect and will fetch the wrong struct/mass. Replacing with patch_ib(ib_marker)%mass fixes an obvious logic bug.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 1090:1090
**Comment:**
*Logic Error: Wrong variable used when computing analytic moment for rectangular geometry: the expression uses `patch_ib(i)%mass` but `i` is undefined here (should use the subroutine argument `ib_marker`); this will read a wrong/undefined mass value and produce incorrect result.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Wrong variable used: patch_ib(i)%mass should be patch_ib(ib_marker)%mass. Variable i is undefined in this context.
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 1090:
<comment>Wrong variable used: `patch_ib(i)%mass` should be `patch_ib(ib_marker)%mass`. Variable `i` is undefined in this context.</comment>
<file context>
@@ -978,6 +1064,78 @@ contains
+ if (patch_ib(ib_marker)%geometry == 2) then ! circle
+ patch_ib(ib_marker)%moment = 0.5 * patch_ib(ib_marker)%mass * (patch_ib(ib_marker)%radius)**2
+ elseif (patch_ib(ib_marker)%geometry == 3) then ! rectangle
+ patch_ib(ib_marker)%moment = patch_ib(i)%mass * (patch_ib(ib_marker)%length_x**2 + patch_ib(ib_marker)%length_y**2) / 6._wp
+ elseif (patch_ib(ib_marker)%geometry == 8) then ! sphere
+ patch_ib(ib_marker)%moment = 0.4 * patch_ib(ib_marker)%mass * (patch_ib(ib_marker)%radius)**2
</file context>
| patch_ib(ib_marker)%moment = patch_ib(i)%mass * (patch_ib(ib_marker)%length_x**2 + patch_ib(ib_marker)%length_y**2) / 6._wp | |
| patch_ib(ib_marker)%moment = patch_ib(ib_marker)%mass * (patch_ib(ib_marker)%length_x**2 + patch_ib(ib_marker)%length_y**2) / 6._wp |
✅ Addressed in a216ed4
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Suggestion: Loop bound typo in the moment-of-inertia grid scan: the inner loop is written do j = 0, j which uses the loop variable j as its own bound and is almost certainly incorrect (will either be a no-op or undefined behavior); it should use the grid size (e.g., n) instead. [logic error]
Severity Level: Minor
| do j = 0, j | |
| do j = 0, n |
Why it matters? ⭐
The inner loop bound "do j = 0, j" is clearly a typo and will not iterate over the intended grid (uses the loop variable as its own bound). Changing it to "0, n" (or appropriate grid size) fixes a logic bug that would otherwise make the loop a no-op or unpredictable.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 1105:1105
**Comment:**
*Logic Error: Loop bound typo in the moment-of-inertia grid scan: the inner loop is written `do j = 0, j` which uses the loop variable `j` as its own bound and is almost certainly incorrect (will either be a no-op or undefined behavior); it should use the grid size (e.g., `n`) instead.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: Loop variable j is used as its own upper bound. This should be do j = 0, n to iterate over the y-dimension grid.
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 1105:
<comment>Loop variable `j` is used as its own upper bound. This should be `do j = 0, n` to iterate over the y-dimension grid.</comment>
<file context>
@@ -978,6 +1064,78 @@ contains
+
+ $:GPU_PARALLEL_LOOP(private='[position,closest_point_along_axis,vector_to_axis,distance_to_axis]', copy='[moment,count]', copyin='[ib_marker,cell_volume,axis]', collapse=3)
+ do i = 0, m
+ do j = 0, j
+ do k = 0, p
+ if (ib_markers%sf(i, j, k) == ib_marker) then
</file context>
| do j = 0, j | |
| do j = 0, n |
✅ Addressed in a4967c9
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: Loop bound error: do j = 0, j uses j as both loop variable and upper bound. This will cause incorrect iteration since j is uninitialized. Should be do j = 0, n to iterate over the y-dimension.
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 1104:
<comment>Loop bound error: `do j = 0, j` uses `j` as both loop variable and upper bound. This will cause incorrect iteration since `j` is uninitialized. Should be `do j = 0, n` to iterate over the y-dimension.</comment>
<file context>
@@ -978,6 +1064,77 @@ contains
+
+ $:GPU_PARALLEL_LOOP(private='[position,closest_point_along_axis,vector_to_axis,distance_to_axis]', copy='[moment,count]', copyin='[ib_marker,cell_volume,axis]', collapse=3)
+ do i = 0, m
+ do j = 0, j
+ do k = 0, p
+ if (ib_markers%sf(i, j, k) == ib_marker) then
</file context>
| do j = 0, j | |
| do j = 0, n |
✅ Addressed in a4967c9
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Incorrect distance calculation: sum(vector_to_axis) sums components, not computing squared distance. Should be sum(vector_to_axis**2) per the comment.
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>Incorrect distance calculation: `sum(vector_to_axis)` sums components, not computing squared distance. Should be `sum(vector_to_axis**2)` per the comment.</comment>
<file context>
@@ -978,6 +1064,78 @@ contains
+ ! project the position along the axis to find the closest distance to the rotation axis
+ closest_point_along_axis = axis*dot_product(axis, position)
+ vector_to_axis = position - closest_point_along_axis
+ distance_to_axis = sum(vector_to_axis) ! saves the distance to the axis squared
+
+ ! compute the position component of the moment
</file context>
| distance_to_axis = sum(vector_to_axis) ! saves the distance to the axis squared | |
| distance_to_axis = sum(vector_to_axis**2) ! saves the distance to the axis squared |
✅ Addressed in a216ed4
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Incorrect distance calculation: sum(vector_to_axis) computes the sum of components, not the squared distance. For moment of inertia, this should be sum(vector_to_axis**2) to get the squared perpendicular distance from the rotation axis.
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 1120:
<comment>Incorrect distance calculation: `sum(vector_to_axis)` computes the sum of components, not the squared distance. For moment of inertia, this should be `sum(vector_to_axis**2)` to get the squared perpendicular distance from the rotation axis.</comment>
<file context>
@@ -978,6 +1064,77 @@ contains
+ ! project the position along the axis to find the closest distance to the rotation axis
+ closest_point_along_axis = axis*dot_product(axis, position)
+ vector_to_axis = position - closest_point_along_axis
+ distance_to_axis = sum(vector_to_axis) ! saves the distance to the axis squared
+
+ ! compute the position component of the moment
</file context>
| distance_to_axis = sum(vector_to_axis) ! saves the distance to the axis squared | |
| distance_to_axis = sum(vector_to_axis**2) ! saves the distance to the axis squared |
✅ Addressed in a216ed4
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -611,6 +611,8 @@ contains | |||||||||||||||||||||||||||||
| if (ib) then | ||||||||||||||||||||||||||||||
| ! check if any IBMS are moving, and if so, update the markers, ghost points, levelsets, and levelset norms | ||||||||||||||||||||||||||||||
| if (moving_immersed_boundary_flag) then | ||||||||||||||||||||||||||||||
| call s_compute_ib_forces(q_prim_vf(E_idx)%sf(0:m, 0:n, 0:p)) ! compute the force and torque on the IB from the fluid | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Unsafe use of Severity Level: Critical 🚨
Suggested change
Why it matters? ⭐This is a valid runtime-safety concern: q_prim_vf is only allocated in initialization when (.not. igr). Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/simulation/m_time_steppers.fpp
**Line:** 614:614
**Comment:**
*Possible Bug: Unsafe use of `q_prim_vf` when computing IB forces: `q_prim_vf` is only allocated in initialization when `.not. igr`. Calling `s_compute_ib_forces(q_prim_vf(E_idx)%sf(...))` without guarding for `igr` can dereference an unallocated array and crash when `igr` is true. Wrap the call in a guard so it's only executed when `q_prim_vf` is available.
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 = 1, num_ibs | ||||||||||||||||||||||||||||||
| if (s == 1) then | ||||||||||||||||||||||||||||||
| patch_ib(i)%step_vel = patch_ib(i)%vel | ||||||||||||||||||||||||||||||
|
|
@@ -621,14 +623,22 @@ contains | |||||||||||||||||||||||||||||
| patch_ib(i)%step_z_centroid = patch_ib(i)%z_centroid | ||||||||||||||||||||||||||||||
| end if | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (patch_ib(i)%moving_ibm == 1) then | ||||||||||||||||||||||||||||||
| do j = 1, 3 | ||||||||||||||||||||||||||||||
| patch_ib(i)%vel(j) = (rk_coef(s, 1)*patch_ib(i)%step_vel(j) + rk_coef(s, 2)*patch_ib(i)%vel(j) + rk_coef(s, 3)*0._wp*dt)/rk_coef(s, 4) ! 0.0 is a placeholder for accelerations | ||||||||||||||||||||||||||||||
| patch_ib(i)%angular_vel(j) = (rk_coef(s, 1)*patch_ib(i)%step_angular_vel(j) + rk_coef(s, 2)*patch_ib(i)%angular_vel(j) + rk_coef(s, 3)*0._wp*dt)/rk_coef(s, 4) | ||||||||||||||||||||||||||||||
| if (patch_ib(i)%moving_ibm > 0) then | ||||||||||||||||||||||||||||||
| patch_ib(i)%vel = (rk_coef(s, 1)*patch_ib(i)%step_vel + rk_coef(s, 2)*patch_ib(i)%vel)/rk_coef(s, 4) | ||||||||||||||||||||||||||||||
| patch_ib(i)%angular_vel = (rk_coef(s, 1)*patch_ib(i)%step_angular_vel + rk_coef(s, 2)*patch_ib(i)%angular_vel)/rk_coef(s, 4) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ! Update the angle of the IB | ||||||||||||||||||||||||||||||
| patch_ib(i)%angles(j) = (rk_coef(s, 1)*patch_ib(i)%step_angles(j) + rk_coef(s, 2)*patch_ib(i)%angles(j) + rk_coef(s, 3)*patch_ib(i)%angular_vel(j)*dt)/rk_coef(s, 4) | ||||||||||||||||||||||||||||||
| end do | ||||||||||||||||||||||||||||||
| if (patch_ib(i)%moving_ibm == 2) then ! if we are using two-way coupling, apply force and torque | ||||||||||||||||||||||||||||||
| ! update the velocity from the force value | ||||||||||||||||||||||||||||||
| patch_ib(i)%vel = patch_ib(i)%vel + rk_coef(s, 3)*dt*(patch_ib(i)%force/patch_ib(i)%mass)/rk_coef(s, 4) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ! update the angular velocity with the torque value | ||||||||||||||||||||||||||||||
| patch_ib(i)%angular_vel = (patch_ib(i)%angular_vel * patch_ib(i)%moment) + (rk_coef(s, 3)*dt*patch_ib(i)%torque/rk_coef(s, 4)) ! add the torque to the angular momentum | ||||||||||||||||||||||||||||||
| call s_compute_moment_of_inertia(i, patch_ib(i)%angular_vel) ! update the moment of inertia to be based on the direction of the angular momentum | ||||||||||||||||||||||||||||||
| patch_ib(i)%angular_vel = patch_ib(i)%angular_vel / patch_ib(i)%moment ! convert back to angular velocity with the new moment of inertia | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| ! update the angular velocity with the torque value | |
| patch_ib(i)%angular_vel = (patch_ib(i)%angular_vel * patch_ib(i)%moment) + (rk_coef(s, 3)*dt*patch_ib(i)%torque/rk_coef(s, 4)) ! add the torque to the angular momentum | |
| call s_compute_moment_of_inertia(i, patch_ib(i)%angular_vel) ! update the moment of inertia to be based on the direction of the angular momentum | |
| patch_ib(i)%angular_vel = patch_ib(i)%angular_vel / patch_ib(i)%moment ! convert back to angular velocity with the new moment of inertia | |
| ! update the angular velocity with the torque value | |
| patch_ib(i)%angular_vel = patch_ib(i)%angular_vel + (rk_coef(s, 3)*dt*patch_ib(i)%torque / patch_ib(i)%moment)/rk_coef(s, 4) | |
| ! The moment of inertia should be updated based on the new orientation of the body, which is not done here. | |
| ! For a rigid body, the moment of inertia tensor in the body-fixed frame is constant. | |
| ! What is needed is to transform it to the lab frame. | |
| ! The call to s_compute_moment_of_inertia here seems to be based on a misunderstanding. | |
| ! If the intent is to handle shape changes or axis of rotation changes, this needs a more robust implementation. | |
| ! For now, removing the re-computation within the time step seems safer. | |
| ! call s_compute_moment_of_inertia(i, patch_ib(i)%angular_vel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Unused import:
mathis imported but never used anywhere; keep dead imports out of the codebase to avoid confusion and maintain clean dependencies — remove the import. [possible bug]Severity Level: Critical 🚨
Why it matters? ⭐
The module imports math but never uses it anywhere in the current PR diff. Removing the import removes dead code and linter warnings and has no functional impact.
Prompt for AI Agent 🤖