Conversation
…on. Ready to collide
Claude Code ReviewHead SHA: a7874f1 Files changed:
Findings1.
|
Review Summary by QodoAdd soft-sphere collision model and restart capability for immersed boundaries
WalkthroughsDescription• Added soft-sphere collision model for immersed boundary particles - Detects collisions between IBs and walls using overlap distances - Detects collisions between IBs using ghost-point neighbor search - Applies normal and tangential forces with friction • Implemented IB restart capability from previous simulation states - Reads IB kinematic state from restart_data/ib_state.dat - Preserves collision history by appending to state file • Separated immersed boundary patch array from initial condition patches - Increased num_ib_patches_max to 50000 for scalability - Reduced num_patches_max to 10 for initial conditions only • Added collision parameters and configuration options - collision_model, coefficient_of_restitution, collision_time, ib_coefficient_of_friction Diagramflowchart LR
A["IB Collision Detection"] -->|wall overlaps| B["Wall Collision Forces"]
A -->|IB-IB overlaps| C["IB Collision Forces"]
B --> D["Apply Forces & Torques"]
C --> D
D --> E["Update IB Motion"]
E --> F["Write IB State File"]
F --> G["Restart from State"]
File Changes1. src/simulation/m_collisions.fpp
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ! if it is outside the domain in one direction, project it somewhere inside so at least one rank owns it | ||
| if (collision_location(${ID}$) < ${X}$_domain%beg) then | ||
| projected_location(${ID}$) = ${X}$_domain%beg | ||
| else if (${X}$_domain%end < collision_location(${ID}$)) then | ||
| projected_location(${ID}$) = ${X}$_domain%end - 1.0e-10 | ||
| end if |
There was a problem hiding this comment.
1. 1.0e-10 missing _wp 📘 Rule violation ≡ Correctness
The new collision code assigns unkinded numeric literals (e.g., 1.0e-10, 0) into real(wp) variables, breaking the project precision discipline and risking inconsistent precision across builds. This violates the requirement to use wp/stp-kinded literals and generic intrinsics consistently.
Agent Prompt
## Issue description
`real(wp)` code in `m_collisions` uses unkinded numeric literals (e.g., `1.0e-10`, `0`), which violates the project precision discipline.
## Issue Context
The code operates on `real(wp)` state (domain bounds, overlaps) and should use kinded literals like `0._wp` / `1.0e-10_wp` to avoid precision inconsistencies.
## Fix Focus Areas
- src/simulation/m_collisions.fpp[31-44]
- src/simulation/m_collisions.fpp[416-421]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| _r("collision_model", INT, {"ib"}) | ||
| _r("coefficient_of_restitution", REAL, {"ib"}) | ||
| _r("collision_time", REAL, {"ib"}) | ||
| _r("ib_coefficient_of_friction", REAL, {"ib"}) |
There was a problem hiding this comment.
2. collision_model lacks validation 📘 Rule violation ☼ Reliability
New collision parameters are introduced but no corresponding physics/validity constraints are enforced in toolchain/mfc/case_validator.py, allowing invalid values (e.g., coefficient_of_restitution <= 0 leading to log(e) issues). This violates the requirement to add new parameters to required validation locations when constraints exist.
Agent Prompt
## Issue description
Collision parameters (`collision_model`, `coefficient_of_restitution`, `collision_time`, `ib_coefficient_of_friction`) are added, but the Python `case_validator.py` does not validate their allowed ranges/values.
## Issue Context
`m_collisions` computes `log(e)` where `e = coefficient_of_restitution`, so invalid `e` (<= 0, > 1) can cause NaNs or undefined behavior. Documentation states allowed ranges/values, implying physics constraints that should be enforced.
## Fix Focus Areas
- toolchain/mfc/params/definitions.py[915-918]
- toolchain/mfc/case_validator.py[554-571]
- docs/documentation/case.md[368-374]
- src/simulation/m_collisions.fpp[33-38]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| integer, parameter :: num_patches_max = 10 !< Maximum number of IC patches | ||
| integer, parameter :: num_ib_patches_max = 50000 !< Maximum number of immersed boundary patches (patch_ib) |
There was a problem hiding this comment.
3. Ic patches capped at 10 🐞 Bug ≡ Correctness
num_patches_max was reduced to 10, capping patch_icpp and other IC-sized arrays to 10 and breaking any case with >10 initial-condition patches (validation and/or runtime array bounds). This also desynchronizes the toolchain’s fallback constants, which still assume 1000 when Fortran sources are unavailable.
Agent Prompt
### Issue description
`num_patches_max` was reduced to 10, which caps **initial condition** patch arrays (`patch_icpp`, `alter_patch`, etc.) to 10 and breaks cases needing more IC patches; toolchain fallback constants also become inconsistent.
### Issue Context
This PR adds a separate `num_ib_patches_max` for IB patches, but `num_patches_max` still governs IC patches.
### Fix Focus Areas
- src/common/m_constants.fpp[24-28]
- toolchain/mfc/params/namelist_parser.py[506-513]
- (If intentional) add explicit release-note/docs update; otherwise restore prior value (e.g., 1000).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| type(ib_patch_parameters), dimension(num_ib_patches_max) :: patch_ib !< Immersed boundary patch parameters | ||
| type(vec3_dt), allocatable, dimension(:) :: airfoil_grid_u, airfoil_grid_l |
There was a problem hiding this comment.
4. Ib checks stop at 10 🐞 Bug ≡ Correctness
The pre_process IB patch validator still loops to num_patches_max, so with the new IC/IB separation and num_patches_max=10 it only validates the first 10 IB patches and can miss invalid IB definitions beyond that. This can allow out-of-range/invalid IB parameters through pre_process checks and fail later at runtime.
Agent Prompt
### Issue description
`m_check_ib_patches` validates IB patches using `do i = 1, num_patches_max`, which now (a) is an IC patch limit and (b) was reduced to 10. As a result, IB patches beyond 10 are not validated.
### Issue Context
`patch_ib` is now dimensioned by `num_ib_patches_max` in pre_process, so validation must follow that bound (or at least validate all `1..num_ibs` and assert the rest are default).
### Fix Focus Areas
- src/pre_process/m_check_ib_patches.fpp[32-71]
- src/common/m_constants.fpp[24-28]
- src/pre_process/m_global_parameters.fpp[169-176]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| NUM_PATCHES_MAX = _fc("num_patches_max") # patch_icpp (Fortran array bound) | ||
| NIB = _fc("num_ib_patches_max") # patch_ib (Fortran array bound) |
There was a problem hiding this comment.
5. Fallback constants missing nib 🐞 Bug ☼ Reliability
The toolchain now requires num_ib_patches_max, but the fallback constants used when src/common/m_constants.fpp isn’t shipped do not define it, causing the parameter registry initialization to throw at runtime. This breaks installed/Homebrew-style toolchain usage.
Agent Prompt
### Issue description
Toolchain startup can fail in environments where `src/common/m_constants.fpp` is not shipped, because fallback constants omit `num_ib_patches_max` while `definitions.py` now requires it.
### Issue Context
`_fc("num_ib_patches_max")` raises when the constant is absent.
### Fix Focus Areas
- toolchain/mfc/params/namelist_parser.py[506-513]
- toolchain/mfc/params/definitions.py[15-32]
### Expected change
Add `"num_ib_patches_max": <Fortran value>` to `_FALLBACK_CONSTANTS` and keep it in sync with `src/common/m_constants.fpp`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ! Temporary array to hold all detected pairs (with potential duplicates) | ||
| integer, dimension(num_gps, 2) :: raw_pairs | ||
| integer :: num_raw, local_num_raw |
There was a problem hiding this comment.
6. Raw_pairs stack allocation 🐞 Bug ☼ Reliability
s_detect_ib_collisions declares raw_pairs(num_gps,2) as an automatic local array, which can overflow stack or cause large per-step overhead for large num_gps. This is called during force evaluation, so it risks hard crashes on large runs.
Agent Prompt
### Issue description
`raw_pairs(num_gps,2)` is an automatic local array and can blow the stack or incur heavy per-timestep allocation costs for large `num_gps`.
### Issue Context
This routine runs in the timestep loop (via force computation).
### Fix Focus Areas
- src/simulation/m_collisions.fpp[227-277]
### Expected change
Move `raw_pairs` to an allocatable module-scope buffer sized to a safe upper bound (or allocate/deallocate on the heap inside the routine), and reuse it across calls.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| normal_vector = centroid_2 - centroid_1 | ||
| overlap_distance = patch_ib(pid1)%radius + patch_ib(pid2)%radius - norm2(normal_vector) | ||
| if (overlap_distance > 0._wp) then ! if the two patches are close enough to collide | ||
| normal_vector = normal_vector/norm2(normal_vector) | ||
| if (f_local_rank_owns_collision(centroid_1)) then |
There was a problem hiding this comment.
7. Zero-distance collision nans 🐞 Bug ☼ Reliability
When two IB centroids coincide, norm2(normal_vector) is zero and the normal normalization divides by zero, producing NaNs that propagate into forces/torques. This can occur from bad initial conditions or restart data and is not guarded against.
Agent Prompt
### Issue description
Normal vector normalization can divide by zero when centroids are identical.
### Issue Context
`overlap_distance > 0` does not imply `norm2(normal_vector) > 0`.
### Fix Focus Areas
- src/simulation/m_collisions.fpp[109-117]
### Expected change
Compute `dist = norm2(normal_vector)` once and:
- if `dist <= eps`, skip collision or choose a stable fallback normal
- else normalize by `dist`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (proc_rank == 0) then | ||
| file_loc = trim(case_dir) // '/restart_data/ib_state.dat' | ||
| inquire (FILE=trim(file_loc), EXIST=file_exist) | ||
| print *, "IB Restart File Exists: ", file_exist | ||
| if (file_exist) then | ||
| open (newunit=file_unit, file=trim(file_loc), form='unformatted', access='stream', status='old', iostat=ios) | ||
| print *, "iostat ", ios | ||
| else | ||
| call s_mpi_abort('Cannot open IB state file for restart: ' // trim(file_loc)) | ||
| end if | ||
|
|
||
| ! Read all records; the last num_ibs records are the final state | ||
| do | ||
| read (file_unit, iostat=ios) time_read, ib_id, force_read, torque_read, vel_read, angular_vel_read, angles_read, & | ||
| & xc_read, yc_read, zc_read | ||
| if (ios /= 0) exit | ||
| patch_ib(ib_id)%vel = vel_read | ||
| patch_ib(ib_id)%angular_vel = angular_vel_read | ||
| patch_ib(ib_id)%angles = angles_read | ||
| patch_ib(ib_id)%x_centroid = xc_read | ||
| patch_ib(ib_id)%y_centroid = yc_read | ||
| patch_ib(ib_id)%z_centroid = zc_read | ||
| end do |
There was a problem hiding this comment.
8. Restart reader out-of-bounds 🐞 Bug ☼ Reliability
s_read_ib_restart_data does not check open() iostat for failure and does not bounds-check ib_id before writing patch_ib(ib_id), risking memory corruption on malformed files or when num_ibs differs between runs. It also prints debug output unconditionally on restart.
Agent Prompt
### Issue description
Restart reader can corrupt memory by writing `patch_ib(ib_id)` without validating `ib_id`, and ignores open() failures.
### Issue Context
Restart files can be incomplete/corrupt; users may also change `num_ibs` between runs.
### Fix Focus Areas
- src/simulation/m_start_up.fpp[1135-1185]
- src/simulation/m_data_output.fpp[919-930]
### Expected change
- After `open(..., iostat=ios)`, abort if `ios /= 0`.
- Before assigning `patch_ib(ib_id)%...`, verify `1 <= ib_id <= num_ibs`; otherwise skip/abort with a clear message.
- Remove unconditional `print *` debug lines or gate behind a verbosity flag.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
📝 WalkthroughWalkthroughThis pull request introduces collision physics modeling for immersed boundary simulations. A new module 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulation/m_ibm.fpp (1)
570-577:⚠️ Potential issue | 🔴 CriticalDon't leave encoded periodic IDs in
ib_markers.
ghost_points_in(local_idx)%ib_patch_idis decoded here, butib_markers%sfnow keeps the encoded periodic-image value. The rest of this module still does raw lookups likepatch_ib(ib_markers%sf(...))ins_ibm_correct_stateands_compute_ib_forces, so periodic ghost cells will pick up the wrong patch state as soon as an IB crosses a periodic boundary.
🧹 Nitpick comments (2)
tests/AA49A8BC/golden-metadata.txt (1)
13-119: Reduce machine-specific metadata in committed golden artifacts.This file records host/local-environment identifiers (e.g., Line 13 hostname, Line 29 absolute user path, Line 119 CPU model). Since metadata is informational and not used for golden comparison, committing these details adds noisy diffs and leaks local context. Prefer excluding this metadata file from versioned test artifacts (or sanitize generation in a follow-up) and keep only validation-relevant golden data.
Based on learnings: Golden files are generated with
./mfc.sh test --generateand should not be manually edited.src/common/m_helper.fpp (1)
300-311: Avoid duplicated cross-product logic betweenf_crossands_cross_product.This is functionally correct, but keeping two separate implementations invites divergence. Reuse
f_crossins_cross_product.Refactor suggestion
subroutine s_cross_product(a, b, c) $:GPU_ROUTINE(parallelism='[seq]') real(wp), intent(in) :: a(3), b(3) real(wp), intent(out) :: c(3) - c(1) = a(2)*b(3) - a(3)*b(2) - c(2) = a(3)*b(1) - a(1)*b(3) - c(3) = a(1)*b(2) - a(2)*b(1) + c = f_cross(a, b) end subroutine s_cross_product
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 701a3269-2add-4226-9ccf-dc2b22957260
📒 Files selected for processing (21)
docs/documentation/case.mddocs/module_categories.jsonexamples/3D_mibm_sphere_head_on_collision/case.pysrc/common/m_constants.fppsrc/common/m_helper.fppsrc/post_process/m_data_input.f90src/pre_process/m_global_parameters.fppsrc/pre_process/m_mpi_proxy.fppsrc/simulation/m_collisions.fppsrc/simulation/m_data_output.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_ib_patches.fppsrc/simulation/m_ibm.fppsrc/simulation/m_mpi_proxy.fppsrc/simulation/m_start_up.fppsrc/simulation/m_time_steppers.fpptests/AA49A8BC/golden-metadata.txttests/AA49A8BC/golden.txttoolchain/mfc/case_validator.pytoolchain/mfc/params/definitions.pytoolchain/mfc/params/descriptions.py
| | `coefficient_of_restitution` | Real | A number 0 to 1 describing how elastic IB collisions are | | ||
| | `collision_model` | Integer | Integer to select the collision model being used for IB collisions. | | ||
| | `collision_time` | Real | Amount of simulation time used to resolve collisions | | ||
| | `ib_coefficient_of_friction` | Real | Coefficient of friction used in IB collisions | |
There was a problem hiding this comment.
Clean up IB collision parameter wording (typos + consistency).
There are several spelling errors and slightly inconsistent range wording in these new parameter docs.
📝 Suggested edit
-- `coefficient_of_restitution` is a number from 0 (exclusive) to 1 (inclusive) describing how elastic IB collisions are. 0 is for perfectly inellastic collisions while 1 is for perfectly ellastic collisions.
+- `coefficient_of_restitution` is a number from 0 (exclusive) to 1 (inclusive) describing how elastic IB collisions are. 0 is perfectly inelastic, while 1 is perfectly elastic.
-- `collision_model` is an integer to select the collision model being used for IB collisions. Using 0 disables collisions and collisiono checking. 1 enables the soft-sphere collision model, where all IBs must be circles or sphere and those IBs can collide with each other as well as walls.
+- `collision_model` is an integer selecting the IB collision model. Using 0 disables collisions and collision checking. Using 1 enables the soft-sphere model, where IBs represented as circles/spheres can collide with each other and walls.
-- `collision_time` is approximately the amount of simulation time used to resolve collisions. This is handled by modifying the spring gonstant used to apply collision forces.
+- `collision_time` is approximately the amount of simulation time used to resolve collisions. This is handled by modifying the spring constant used to apply collision forces.Also applies to: 368-374
| "patch_ib(1)%x_centroid": -1.0 * lead_distance * math.sin(collision_angle_radians), # get a lead up distance to the collision | ||
| "patch_ib(1)%y_centroid": radius + lead_distance * math.sin(collision_angle_radians), | ||
| "patch_ib(1)%z_centroid": 0.0, | ||
| "patch_ib(1)%radius": radius, | ||
| "patch_ib(1)%slip": "F", | ||
| "patch_ib(1)%mass": 1.0e6, # arbitrarily high mass to ignore fluid | ||
| "patch_ib(1)%vel(1)": velocity * math.sin(collision_angle_radians), | ||
| "patch_ib(1)%vel(2)": -velocity * math.cos(collision_angle_radians), |
There was a problem hiding this comment.
Use cos(theta) for the initial wall-normal offset.
vel(1)=velocity*sin(theta) and vel(2)=-velocity*cos(theta) define the approach direction. Backing the sphere away by lead_distance from first contact therefore needs a wall-normal offset of lead_distance*cos(theta), not lead_distance*sin(theta). As written, the example reaches the wall sooner than the simulation_time = 2 * lead_distance / velocity + collision_time derivation assumes, so the rebound-angle validation is measuring the wrong setup.
🛠️ Minimal fix
- "patch_ib(1)%y_centroid": radius + lead_distance * math.sin(collision_angle_radians),
+ "patch_ib(1)%y_centroid": radius + lead_distance * math.cos(collision_angle_radians),| integer, parameter :: num_patches_max = 10 !< Maximum number of IC patches | ||
| integer, parameter :: num_ib_patches_max = 50000 !< Maximum number of immersed boundary patches (patch_ib) |
There was a problem hiding this comment.
Restore num_patches_max to avoid breaking existing multi-patch cases.
Line 26 drops the IC patch cap from 1000 to 10, which will reject or break many valid cases unrelated to IB collisions. Keep the legacy IC cap and use the new IB-specific cap only for patch_ib.
Proposed fix
- integer, parameter :: num_patches_max = 10 !< Maximum number of IC patches
+ integer, parameter :: num_patches_max = 1000 !< Maximum number of IC patches
integer, parameter :: num_ib_patches_max = 50000 !< Maximum number of immersed boundary patches (patch_ib)📝 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.
| integer, parameter :: num_patches_max = 10 !< Maximum number of IC patches | |
| integer, parameter :: num_ib_patches_max = 50000 !< Maximum number of immersed boundary patches (patch_ib) | |
| integer, parameter :: num_patches_max = 1000 !< Maximum number of IC patches | |
| integer, parameter :: num_ib_patches_max = 50000 !< Maximum number of immersed boundary patches (patch_ib) |
| integer, allocatable, dimension(:,:) :: collision_lookup | ||
| real(wp), allocatable, dimension(:,:) :: wall_overlap_distances | ||
| real(wp) :: spring_stiffness, damping_parameter | ||
| $:GPU_DECLARE(create='[spring_stiffness, damping_parameter]') |
There was a problem hiding this comment.
Add the collision arrays to GPU_DECLARE.
collision_lookup and wall_overlap_distances are both pushed to the device and then read inside GPU kernels, but only the scalar coefficients are in the module device-data declaration. On accelerator builds that leaves these allocatables outside the expected device lifetime.
Suggested change
- $:GPU_DECLARE(create='[spring_stiffness, damping_parameter]')
+ $:GPU_DECLARE(create='[collision_lookup, wall_overlap_distances, spring_stiffness, damping_parameter]')Based on learnings, GPU array lifecycle: declare with $:GPU_DECLARE(create='[...]'), allocate with @:ALLOCATE(), setup pointers with @:ACC_SETUP_SFs() or @:ACC_SETUP_VFs(), deallocate with @:DEALLOCATE().
| @:ALLOCATE(collision_lookup(num_ibs * (num_ibs-1) / 2, 4)) | ||
| @:ALLOCATE(wall_overlap_distances(num_ibs, 6)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the overall structure and key sections of m_collisions.fpp
head -60 src/simulation/m_collisions.fpp | cat -nRepository: MFlowCode/MFC
Length of output: 2683
🏁 Script executed:
# Look at lines 279-280 and surrounding context
sed -n '270,290p' src/simulation/m_collisions.fpp | cat -nRepository: MFlowCode/MFC
Length of output: 1089
🏁 Script executed:
# Check integer declarations and num_ibs constraints
rg -n "integer.*collision_lookup|integer.*wall_overlap|integer.*num_ibs" src/simulation/m_collisions.fpp | head -20Repository: MFlowCode/MFC
Length of output: 121
🏁 Script executed:
# Check the integer kind used in the file
rg -n "implicit none|integer\s*::" src/simulation/m_collisions.fpp | head -30Repository: MFlowCode/MFC
Length of output: 823
🏁 Script executed:
# Find num_ibs definition and constraints
rg -n "num_ibs" src/simulation/m_collisions.fpp | head -20Repository: MFlowCode/MFC
Length of output: 571
🏁 Script executed:
# Search for num_ibs in global parameters
rg -n "num_ibs" src/common/m_global_parameters.fpp | head -20Repository: MFlowCode/MFC
Length of output: 131
🏁 Script executed:
# Look at how collision_lookup is actually used/indexed
rg -n "collision_lookup" src/simulation/m_collisions.fpp -B 1 -A 1Repository: MFlowCode/MFC
Length of output: 1808
🏁 Script executed:
# Check the context around where collision_lookup is zeroed and populated
sed -n '275,330p' src/simulation/m_collisions.fpp | cat -nRepository: MFlowCode/MFC
Length of output: 3167
🏁 Script executed:
# Check if there's any validation on num_ibs
rg -n "num_ibs.*>" src/ | grep -E "(constraint|max|limit|check)" | head -10Repository: MFlowCode/MFC
Length of output: 39
🏁 Script executed:
# Look for num_ibs constraints and documentation
fd -t f "case_validator|parameters" src/common docs/ 2>/dev/null | head -20Repository: MFlowCode/MFC
Length of output: 79
🏁 Script executed:
# Search for num_ibs limits/constraints in documentation or validation
rg -i "num_ibs|maximum.*ib|ib.*limit" docs/ | head -20Repository: MFlowCode/MFC
Length of output: 156
🏁 Script executed:
# Verify the integer overflow calculation more carefully
python3 << 'EOF'
import math
# 32-bit signed integer max
INT_MAX_32 = 2**31 - 1
# Find the threshold where n*(n-1)/2 exceeds INT_MAX_32
# Solving: n^2 - n > 2*INT_MAX_32
# n = (1 + sqrt(1 + 8*INT_MAX_32)) / 2
n_threshold = (1 + math.sqrt(1 + 8*INT_MAX_32)) / 2
print(f"32-bit integer overflow threshold: {n_threshold:.0f}")
# Check specific values
for test_n in [46341, 46342, 65574, 65575]:
result = test_n * (test_n - 1) // 2
fits = "FITS" if result <= INT_MAX_32 else "OVERFLOWS"
print(f" n={test_n}: {test_n}*({test_n}-1)/2 = {result:,} [{fits}]")
EOFRepository: MFlowCode/MFC
Length of output: 315
Correct the integer kind for collision_lookup and reconsider the O(n²) allocation strategy.
The default 32-bit integer overflows at num_ibs ≈ 65,536, not 46,341. However, the core issue remains: allocating num_ibs * (num_ibs-1) / 2 rows (line 40) and zeroing the entire array every step (line 280) creates O(n²) memory allocation and access. Since only num_considered_collisions rows are actually populated, this table should either use a 1D array with dynamic resizing or be bounded by a reasonable upper limit on detected pairs rather than the maximum possible pair count.
| ! | ||
| if (ib) then | ||
| if (moving_immersed_boundary_flag) call s_wrap_periodic_ibs() | ||
| if (ib_state_wrt .and. (.not. moving_immersed_boundary_flag)) then | ||
| call s_compute_ib_forces(q_prim_vf, fluid_pp) | ||
| end if |
There was a problem hiding this comment.
Refresh q_prim_vf before the post-RK IB force sample.
By the time this block runs, q_cons_ts(1)%vf has already been advanced through the last RK stage, but q_prim_vf still reflects the state from the last s_compute_rhs(...) call. s_compute_ib_forces() reads q_prim_vf directly, so the force/torque snapshot computed here is one stage behind unless you reconvert the final conservative state first.
| num_ib_patches_max = get_fortran_constants().get("num_ib_patches_max", 100000) | ||
| self.prohibit( | ||
| ib and num_ibs > num_patches_max, | ||
| f"num_ibs must be <= {num_patches_max} (num_patches_max in m_constants.fpp)", | ||
| ib and num_ibs > num_ib_patches_max, | ||
| f"num_ibs must be <= {num_ib_patches_max} (num_ib_patches_max in m_constants.fpp)", |
There was a problem hiding this comment.
Align num_ib_patches_max fallback with Fortran constant.
The fallback is 100000, but this PR defines num_ib_patches_max as 50000. When constants are unavailable at runtime, validation can incorrectly allow cases that exceed allocated IB capacity.
Proposed fix
- num_ib_patches_max = get_fortran_constants().get("num_ib_patches_max", 100000)
+ num_ib_patches_max = get_fortran_constants().get("num_ib_patches_max", 50000)📝 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.
| num_ib_patches_max = get_fortran_constants().get("num_ib_patches_max", 100000) | |
| self.prohibit( | |
| ib and num_ibs > num_patches_max, | |
| f"num_ibs must be <= {num_patches_max} (num_patches_max in m_constants.fpp)", | |
| ib and num_ibs > num_ib_patches_max, | |
| f"num_ibs must be <= {num_ib_patches_max} (num_ib_patches_max in m_constants.fpp)", | |
| num_ib_patches_max = get_fortran_constants().get("num_ib_patches_max", 50000) | |
| self.prohibit( | |
| ib and num_ibs > num_ib_patches_max, | |
| f"num_ibs must be <= {num_ib_patches_max} (num_ib_patches_max in m_constants.fpp)", |
| "collision_model": { | ||
| "when_set": { | ||
| "requires": ["ib", "coefficient_of_restitution", "collision_time"], | ||
| } | ||
| }, |
There was a problem hiding this comment.
Use when_value for collision_model dependencies.
when_set also fires for collision_model = 0, so an explicit “collisions off” input still has to provide ib, restitution, and collision time. Gate these requirements on the enabled model IDs instead, and this is also the right place to hang the numeric validator checks for the new collision knobs.
As per coding guidelines, "New parameters in toolchain/mfc/params/definitions.py must have correct types, constraints, and tags; validation in case_validator.py must cover interdependencies."
| attrs=_ib_attrs, | ||
| tags=_ib_tags, | ||
| max_index=NUM_PATCHES_MAX, | ||
| max_index=NIB, |
There was a problem hiding this comment.
Propagate NIB into the num_ibs constraint too.
patch_ib is now bounded by num_ib_patches_max, but CONSTRAINTS["num_ibs"] is still unbounded. That lets the toolchain accept num_ibs > NIB, and the Fortran loops over patch_ib(1:num_ibs) will walk past the array bound at runtime.
As per coding guidelines, "New parameters in toolchain/mfc/params/definitions.py must have correct types, constraints, and tags; validation in case_validator.py must cover interdependencies."
| "coefficient_of_restitution": "Real number describing the elasticity of collisions from 0 (perfectly ineleastic) to 1 (perfectly elastic)", | ||
| "collision_model": "Integer selecting the collision model being used. 0 for no collision. 1 for soft-sphere collisions", | ||
| "collision_time": "Amount of simulation time each collision will take to resolve", | ||
| "ib_coefficient_of_friction": "coefficient of friction used in IB collisions", |
There was a problem hiding this comment.
Fix typos and wording in new parameter descriptions.
The new descriptions include misspellings and inconsistent phrasing in user-facing text.
✏️ Proposed text cleanup
- "coefficient_of_restitution": "Real number describing the elasticity of collisions from 0 (perfectly ineleastic) to 1 (perfectly elastic)",
- "collision_model": "Integer selecting the collision model being used. 0 for no collision. 1 for soft-sphere collisions",
- "collision_time": "Amount of simulation time each collision will take to resolve",
- "ib_coefficient_of_friction": "coefficient of friction used in IB collisions",
+ "coefficient_of_restitution": "Real number describing collision elasticity from 0 (perfectly inelastic) to 1 (perfectly elastic)",
+ "collision_model": "Integer selecting the collision model: 0 disables collisions, 1 enables soft-sphere collisions",
+ "collision_time": "Approximate simulation time used to resolve each collision",
+ "ib_coefficient_of_friction": "Coefficient of friction used in IB collisions",📝 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.
| "coefficient_of_restitution": "Real number describing the elasticity of collisions from 0 (perfectly ineleastic) to 1 (perfectly elastic)", | |
| "collision_model": "Integer selecting the collision model being used. 0 for no collision. 1 for soft-sphere collisions", | |
| "collision_time": "Amount of simulation time each collision will take to resolve", | |
| "ib_coefficient_of_friction": "coefficient of friction used in IB collisions", | |
| "coefficient_of_restitution": "Real number describing collision elasticity from 0 (perfectly inelastic) to 1 (perfectly elastic)", | |
| "collision_model": "Integer selecting the collision model: 0 disables collisions, 1 enables soft-sphere collisions", | |
| "collision_time": "Approximate simulation time used to resolve each collision", | |
| "ib_coefficient_of_friction": "Coefficient of friction used in IB collisions", |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1348 +/- ##
==========================================
- Coverage 64.88% 64.76% -0.13%
==========================================
Files 70 71 +1
Lines 18251 18441 +190
Branches 1508 1524 +16
==========================================
+ Hits 11842 11943 +101
- Misses 5446 5531 +85
- Partials 963 967 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…that I am accessing the device copy of positions
1 participant
Description
Added a soft-sphere collision model for sphere/circle IB patches to collide with each other and walls. Collisions with friction were validated against references to obtain reasonable results. This code was also run on 64 MPI ranks to simulate a 10k particle case in a shock tube. Adding this feature also required the ability to restart from previous data, which was added and integrated with existing file reading. That required some refactoring of the existing IB state writing, and will require more modifications to write in parallel.
Fixes #1108
Type of change
Testing
Ran a 10k particle case as well as the new example to compare rebound angles and collision energies.
Checklist
See the developer guide for full coding standards.
AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR:
@coderabbitai review— incremental review (new changes only)@coderabbitai full review— full review from scratch/review— Qodo review/improve— Qodo code suggestions@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label