Conversation
…o levelset-refactor
…o levelset-refactor
…solves the CI bug
|
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
|
CodeAnt AI Incremental review completed. |
|
Thanks for the response. It isn't the level set. It is either model reading or in marker generation. Because if you manually go into the in_patch file and comment out on marker generation for models, you get this exact but. So for one reason or another, it just never generates a single in marker and therefore never gets to the level set, but that's the most I know for certain. I am doing models on the CPU because they are the only patch not supported by GPU compute. The model data structure is causing problems. So instead I opted to just do host to device copies on that one case. I'd have thought that would be acceptable, and it is for NVHPC and Cray. But maybe AMD is off. I will go through that more closely in a bit. My current theory is that the random number generator in the runners is broken unless it is seeded in the program and it's causing some spurious behavior. I just pushed that, so if that's the issue then we will see in a day or so. Thanks for the time.
|
1 similar comment
|
Thanks for the response. It isn't the level set. It is either model reading or in marker generation. Because if you manually go into the in_patch file and comment out on marker generation for models, you get this exact but. So for one reason or another, it just never generates a single in marker and therefore never gets to the level set, but that's the most I know for certain. I am doing models on the CPU because they are the only patch not supported by GPU compute. The model data structure is causing problems. So instead I opted to just do host to device copies on that one case. I'd have thought that would be acceptable, and it is for NVHPC and Cray. But maybe AMD is off. I will go through that more closely in a bit. My current theory is that the random number generator in the runners is broken unless it is seeded in the program and it's causing some spurious behavior. I just pushed that, so if that's the issue then we will see in a day or so. Thanks for the time.
|
|
@danieljvickers that should be fine but your order of operations seems to be incorrect, you should do an update to device only for the STL cases immediately after s_apply_ib_patches (or s_ib_model). Then it should complete the MPI transfer using populate_ib_buffers, and then finally the update to device on line 113 in m_ibm.fpp should be changed to update to host since you already have the correct values on device. There could be potential implicit copies to host in NVHPC/CRAY that are preventing this from causing problems but I would def fix this and try again |
|
@anandrdbz I do not believe this order is incorrect. The subroutine s_apply_ib_patches takes a copy of the host-side in markets array, which is initialized to be all zero on the line before. It performs a copy of this array inside the subroutine for the kernel, which makes all of the IB markers on the CPU after execution. I then send over MPI to fill the halo regions. Up to this point, some compute was done on the GPU, but all results are on the host. That is why a device update is required then. I know this seems odd, but I was forced to do it this way because the subroutine was previously split between preprocess and sim. I plan to correct this after this refactor goes through. Also the model subroutine is CPU based anyways. So all of the compute is CPU and the device update is just an afterthought that doesn't matter until level sets are computed. Please telle of this doesn't make sense, because I'm perhaps missing something. |
|
@danieljvickers but MPI transfer function assumes it's on GPUs and not CPUs ? I did miss the copy function (thought it was only copyin first) , so it seems like after apply_ib_pathes every case should have the correct value on host, but the STL cases won't have it on device. So the update to device should be done before the call to MPI I believe. This still won't cause problems in the test suite since there is no multirank STL case, but I think this is the right order. Check Line 329 in m_mpi_proxy.fpp in subroutine s_mpi_sendrecv_ib_buffers, it assumes everything's on the GPU (which should be fine in general but not for STL) Regarding this particular case though seems like the failure is random if you can't reproduce it. |
|
@anandrdbz that may be possible. I will investigate when I return Either way, as you said that is not the source of error. The problem here is that when only the AMD test runs it fails on the runner and not locally. And the specific error looks like no IB markers get made in the center of the domain on one rank. I am curious if there is something special about the AMD runner environment or something |
|
@danieljvickers I was able to reproduce the error on frontier on an interactive node (I reran it 3 times to make sure it's not random) Make sure you've deleted the build directory and build from scratch using these commands |
|
Oh that's interesting. You can see earlier that I was able to run without changes... But I did miss the --comoyer flag. Maybe that has done it. I'll try in a bit |
|
Oh that's interesting. You can see earlier that I was able to run without changes... But I did miss the --computer flag. Maybe that has done it. I'll try in a bit |
|
Frontier has been sporadically in maintenance yesterday and today. Sometimes allowing login, others not. Could be part of it. |
|
Yeah I will take a look at it tonight on frontier. @anandrdbz I don't think your suggestion is relevant to this bug, but I think it is relevant to a different bug I've been working on today that has been in the code for a while. We will have to wait fore to see |
|
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
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| @:DEALLOCATE(ib_markers%sf) | ||
| @:DEALLOCATE(levelset%sf) | ||
| @:DEALLOCATE(levelset_norm%sf) | ||
| if (allocated(airfoil_grid_u)) then |
There was a problem hiding this comment.
Suggestion: Add a deallocation of models in the finalize routine to match its allocation and prevent a memory leak. [custom_rule]
Severity Level: Minor
| if (allocated(airfoil_grid_u)) then | |
| @:DEALLOCATE(models) |
Why it matters? ⭐
The PR introduces an allocation of models in s_initialize_ibm_module but the finalize routine does not free it — this matches a documented PR-pattern trigger ("PR adds @:ALLOCATE calls without matching @:DEALLOCATE in the corresponding finalization subroutine"). Adding @:DEALLOCATE(models) directly addresses that rule and prevents a memory leak. The proposed change is minimal and aligns with existing deallocation patterns in the file.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 1183:1183
**Comment:**
*Custom Rule: Add a deallocation of `models` in the finalize routine to match its allocation and prevent a memory leak.
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 patch_id = 1, num_ibs | ||
| if (patch_ib(patch_id)%geometry == 5 .or. patch_ib(patch_id)%geometry == 12) then | ||
| allocate (models(patch_id)%model) |
There was a problem hiding this comment.
Suggestion: The allocatable module array holding STL models is never allocated before individual elements are accessed and their model component is allocated, which will cause a runtime error when models(patch_id) is referenced if models itself has not been allocated; you should allocate models to size num_ibs before entering the loop. [possible bug]
Severity Level: Critical 🚨
- ❌ STL immersed-boundary cases crash during model instantiation.
- ❌ All geometry 5/12 patches unusable in simulations.
- ⚠️ Blocks testing of new STL refactor functionality.| allocate (models(patch_id)%model) | |
| if (.not. allocated(models)) then | |
| allocate(models(1:num_ibs)) | |
| end if | |
Steps of Reproduction ✅
1. Configure an immersed boundary patch with STL geometry by setting
`patch_ib(patch_id)%geometry` to 5 (2D STL) or 12 (3D STL); these values are checked in
`s_instantiate_STL_models` at `src/simulation/m_ib_patches.fpp:145-146`.
2. Start a simulation that calls `s_instantiate_STL_models()` (defined at
`src/simulation/m_ib_patches.fpp:120`) during initialization before any other code
allocates the module array `models(:)` (declared at
`src/simulation/m_ib_patches.fpp:58-60`).
3. Execution reaches `allocate (models(patch_id)%model)` at
`src/simulation/m_ib_patches.fpp:147`, which indexes `models(patch_id)` even though the
allocatable array `models(:)` itself has never been allocated.
4. At this point the Fortran runtime attempts to access an unallocated allocatable array,
leading to a runtime error (typically segmentation fault or "allocatable array is not
allocated") and preventing any STL-based immersed boundary setup from running.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ib_patches.fpp
**Line:** 145:145
**Comment:**
*Possible Bug: The allocatable module array holding STL models is never allocated before individual elements are accessed and their `model` component is allocated, which will cause a runtime error when `models(patch_id)` is referenced if `models` itself has not been allocated; you should allocate `models` to size `num_ibs` before entering the loop.
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 (.not. allocated(airfoil_grid_u)) then | ||
| allocate (airfoil_grid_u(1:Np)) | ||
| allocate (airfoil_grid_l(1:Np)) | ||
| @:ALLOCATE(airfoil_grid_u(1:Np)) | ||
| @:ALLOCATE(airfoil_grid_l(1:Np)) | ||
|
|
There was a problem hiding this comment.
Suggestion: In the 2D airfoil routine the airfoil grid arrays are only allocated once and never resized, but Np is recomputed per patch and used in indexing and loop bounds; if a later airfoil patch needs a larger Np than the first, the code will index past the end of airfoil_grid_u/l, causing out-of-bounds access and incorrect geometry, so the arrays should be reallocated when their required size changes. [possible bug]
Severity Level: Critical 🚨
- ❌ Multi-airfoil 2D setups can crash or corrupt memory.
- ❌ Incorrect immersed-boundary markers for later 2D airfoils.
- ⚠️ Results unreliable for complex 2D airfoil configurations.| if (.not. allocated(airfoil_grid_u)) then | |
| allocate (airfoil_grid_u(1:Np)) | |
| allocate (airfoil_grid_l(1:Np)) | |
| @:ALLOCATE(airfoil_grid_u(1:Np)) | |
| @:ALLOCATE(airfoil_grid_l(1:Np)) | |
| if (.not. allocated(airfoil_grid_u) .or. size(airfoil_grid_u) /= Np) then | |
| if (allocated(airfoil_grid_u)) then | |
| deallocate(airfoil_grid_u) | |
| end if | |
| if (allocated(airfoil_grid_l)) then | |
| deallocate(airfoil_grid_l) | |
| end if | |
Steps of Reproduction ✅
1. Define at least two 2D immersed-boundary patches with airfoil geometry by setting
`patch_ib(i)%geometry == 4` for multiple `i` in the case configuration; these are iterated
in `s_apply_ib_patches` at `src/simulation/m_ib_patches.fpp:98-110`, which calls
`s_ib_airfoil` for each such patch.
2. Choose patch parameters so that `Np = Np1+Np2+1` (computed from `ca_in`, `pa`, and
`dx(0)` at `src/simulation/m_ib_patches.fpp:315-317`) for the second airfoil patch is
larger than for the first (e.g. larger chord `c` or different `p`).
3. Run the simulation so that `s_ib_airfoil` (defined at
`src/simulation/m_ib_patches.fpp:292-437`) is invoked first for patch 1:
`airfoil_grid_u/l` are unallocated, so the block at `315-380` allocates them with size
`1:Np_first` and fills them with airfoil coordinates.
4. When `s_ib_airfoil` is called for patch 2, `Np` is recomputed (now `Np_second >
Np_first`), but `airfoil_grid_u` and `airfoil_grid_l` are already allocated, so the
allocation block at `315-380` is skipped; later in the marker loop at
`src/simulation/m_ib_patches.fpp:379-435`, the search `do while (airfoil_grid_u(k)%x <
xy_local(1) .and. k <= Np)` can increment `k` up to `Np_second`, causing out-of-bounds
access on arrays of size `Np_first` and leading to memory corruption or a runtime crash
with incorrect IB marker placement.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_ib_patches.fpp
**Line:** 320:323
**Comment:**
*Possible Bug: In the 2D airfoil routine the airfoil grid arrays are only allocated once and never resized, but `Np` is recomputed per patch and used in indexing and loop bounds; if a later airfoil patch needs a larger `Np` than the first, the code will index past the end of `airfoil_grid_u/l`, causing out-of-bounds access and incorrect geometry, so the arrays should be reallocated when their required size changes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/simulation/m_ibm.fpp">
<violation number="1" location="src/simulation/m_ibm.fpp:114">
P2: patch_ib is modified after its device copy is updated, so subsequent GPU levelset kernels will see stale centroid/offset data. Add a device update after s_compute_centroid_offset to keep GPU state consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (patch_ib(i)%moving_ibm /= 0) call s_compute_centroid_offset(i) ! offsets are computed after IB markers are generated | ||
| end do | ||
|
|
There was a problem hiding this comment.
P2: patch_ib is modified after its device copy is updated, so subsequent GPU levelset kernels will see stale centroid/offset data. Add a device update after s_compute_centroid_offset to keep GPU state consistent.
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 114:
<comment>patch_ib is modified after its device copy is updated, so subsequent GPU levelset kernels will see stale centroid/offset data. Add a device update after s_compute_centroid_offset to keep GPU state consistent.</comment>
<file context>
@@ -109,8 +107,12 @@ contains
+ call s_populate_ib_buffers()
+ $:GPU_UPDATE(host='[ib_markers%sf]')
+ do i = 1, num_ibs
+ if (patch_ib(i)%moving_ibm /= 0) call s_compute_centroid_offset(i) ! offsets are computed after IB markers are generated
+ end do
</file context>
| if (patch_ib(i)%moving_ibm /= 0) call s_compute_centroid_offset(i) ! offsets are computed after IB markers are generated | |
| end do | |
| do i = 1, num_ibs | |
| if (patch_ib(i)%moving_ibm /= 0) call s_compute_centroid_offset(i) ! offsets are computed after IB markers are generated | |
| end do | |
| $:GPU_UPDATE(device='[patch_ib(1:num_ibs)]') |
|
Would I be able to get approval for benchmark on this? |
|
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? |
3 participants
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 levelsets at runtime and simplify immersed-boundary I/O
What Changed
Impact
✅ Much lower memory use for simulations with STLs or multiple immersed boundaries✅ Restart files no longer include bulky levelset arrays (smaller restart size)✅ IB markers available immediately in simulation outputs (including t=0)💡 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.