-
Notifications
You must be signed in to change notification settings - Fork 132
Levelset refactor #1123
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?
Levelset refactor #1123
Changes from all commits
89a0ac5
ef446f5
2f9b1be
e3495d9
f23eaff
59c8dc2
b7b8d2d
128562b
3e097d7
285999f
455d16b
d5aa186
ed9edac
4fa238a
97c54f3
0662d47
75ddc11
b2a821c
6c45fc6
2b84f06
e8fd89f
fb9dbaf
a9c6c5c
31799e1
070e062
8c4fa75
12256ec
0497b85
2a0fb21
d7bef7f
8427e56
7980cb5
1781f45
30dd642
4d69e5a
13cfbbd
aafff97
cc67478
b56678e
dbef122
b7978a3
d8432f6
e7e7d33
b7b6284
05915fb
51bc07f
c304b37
6d7f39c
05c069e
d363ed6
4e0553b
0002855
dc65fc7
668604e
3eb260d
5c9ff7e
1701344
7a9a435
47c9ced
baca16e
20774c6
3769f12
5b5f170
0975fd3
76ab0db
ccca21f
89412b6
c0c9ea3
99de417
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -180,8 +180,18 @@ module m_derived_types | |||||||||||||||||||||||||||||||||
| type :: t_model | ||||||||||||||||||||||||||||||||||
| integer :: ntrs ! Number of triangles | ||||||||||||||||||||||||||||||||||
| type(t_triangle), allocatable :: trs(:) ! Triangles | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| end type t_model | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| type :: t_model_array | ||||||||||||||||||||||||||||||||||
| type(t_model), allocatable :: model | ||||||||||||||||||||||||||||||||||
| real(wp), allocatable, dimension(:, :, :) :: boundary_v | ||||||||||||||||||||||||||||||||||
| real(wp), allocatable, dimension(:, :) :: interpolated_boundary_v | ||||||||||||||||||||||||||||||||||
| integer :: boundary_edge_count | ||||||||||||||||||||||||||||||||||
| integer :: total_vertices | ||||||||||||||||||||||||||||||||||
| logical :: interpolate | ||||||||||||||||||||||||||||||||||
| end type t_model_array | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+186
to
+193
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. 🧩 Analysis chain🏁 Script executed: # First, let's examine the exact code at lines 186-193 in m_derived_types.fpp
sed -n '186,193p' src/common/m_derived_types.fppRepository: MFlowCode/MFC Length of output: 395 🏁 Script executed: # Find and examine m_compute_levelset to see how it uses these components
fd -e fpp -e f90 | xargs grep -l "m_compute_levelset" | head -5Repository: MFlowCode/MFC Length of output: 119 🏁 Script executed: # Search for pointer association patterns with boundary_v or interpolated_boundary_v
rg "boundary_v\s*=>" src/Repository: MFlowCode/MFC Length of output: 258 🏁 Script executed: # Check if TARGET is already on allocatable components in the actual file
rg -A 10 "type :: t_model_array" src/common/m_derived_types.fppRepository: MFlowCode/MFC Length of output: 553 🏁 Script executed: # Examine the context around pointer associations in m_compute_levelset
rg -B 5 -A 5 "boundary_v => models" src/simulation/m_compute_levelset.fppRepository: MFlowCode/MFC Length of output: 577 🏁 Script executed: # Look for pointer declarations in m_compute_levelset
rg "pointer.*::.*boundary_v" src/simulation/m_compute_levelset.fppRepository: MFlowCode/MFC Length of output: 186 🏁 Script executed: # Get broader context - find the subroutine containing these assignments
rg -B 20 "boundary_v => models" src/simulation/m_compute_levelset.fpp | head -60Repository: MFlowCode/MFC Length of output: 926 🏁 Script executed: # Check if other allocatable components in derived types use TARGET
rg "allocatable.*target|target.*allocatable" src/common/m_derived_types.fppRepository: MFlowCode/MFC Length of output: 39 🏁 Script executed: # Search for other derived type definitions with allocatable components
rg -A 8 "type ::" src/common/m_derived_types.fpp | grep -A 8 "allocatable" | head -40Repository: MFlowCode/MFC Length of output: 479 🏁 Script executed: # Check the coding guidelines file to see if TARGET is required for allocatable in derived types
cat .cursor/rules/mfc-agent-rules.mdc | head -100Repository: MFlowCode/MFC Length of output: 3968 🏁 Script executed: # Check if models (the array of t_model_array) is declared with TARGET
rg -B 5 -A 5 "models.*t_model_array|allocatable.*models" src/simulation/m_compute_levelset.fpp | head -40Repository: MFlowCode/MFC Length of output: 39 🏁 Script executed: # Search for how models is declared - find the variable declaration
rg "type.*t_model_array.*::|:.*models" src/simulation/m_compute_levelset.fppRepository: MFlowCode/MFC Length of output: 39 🏁 Script executed: # Look for the full context of where models is declared/allocated
rg -B 10 "models(patch_id)" src/simulation/m_compute_levelset.fpp | head -60Repository: MFlowCode/MFC Length of output: 39 🏁 Script executed: # Find where models is passed as a parameter or declared in m_compute_levelset
rg "subroutine.*models|function.*models" src/simulation/m_compute_levelset.fppRepository: MFlowCode/MFC Length of output: 39 🏁 Script executed: # Look for all subroutine/function signatures that use models
ast-grep --pattern 'subroutine $FUNC($$$models$$$)
$$$
end subroutine'Repository: MFlowCode/MFC Length of output: 39 🏁 Script executed: # Find the subroutine containing the pointer associations
rg -B 50 "boundary_v => models" src/simulation/m_compute_levelset.fpp | grep "subroutine\|function" | tail -1Repository: MFlowCode/MFC Length of output: 92 🏁 Script executed: # Get the full subroutine signature to see if models is a parameter
sed -n '/subroutine.*compute_levelset/,/end subroutine/p' src/simulation/m_compute_levelset.fpp | head -80Repository: MFlowCode/MFC Length of output: 39 🏁 Script executed: # Find the full subroutine s_model_levelset with its signature and initial context
rg -A 60 "subroutine s_model_levelset" src/simulation/m_compute_levelset.fpp | head -80Repository: MFlowCode/MFC Length of output: 2385 🏁 Script executed: # Check what type gp is and how models is accessed (likely via module-level variable)
rg "type.*::|intent.*models" src/simulation/m_compute_levelset.fpp | head -20Repository: MFlowCode/MFC Length of output: 572 🏁 Script executed: # Search for module-level declarations in m_compute_levelset
rg "^[[:space:]]*(type|real|integer|logical|character).*models|^[[:space:]]*allocatable.*models" src/simulation/m_compute_levelset.fppRepository: MFlowCode/MFC Length of output: 39 🏁 Script executed: # Look at the top of m_compute_levelset module for variable declarations
head -100 src/simulation/m_compute_levelset.fppRepository: MFlowCode/MFC Length of output: 3212 🏁 Script executed: # Search for where models is declared or imported - it's used but not a parameter
rg "models|allocate.*models|type.*models" src/simulation/m_compute_levelset.fpp | grep -v "patch_models\|s_model_\|f_model_\|t_model"Repository: MFlowCode/MFC Length of output: 440 🏁 Script executed: # Search in m_ib_patches which is used in m_compute_levelset
rg "models" src/common/m_ib_patches.fpp | head -20Repository: MFlowCode/MFC Length of output: 124 🏁 Script executed: # Look at m_global_parameters to see if models is declared there
rg "models" src/common/m_global_parameters.fpp | head -20Repository: MFlowCode/MFC Length of output: 131 🏁 Script executed: # Search broadly for where models array is declared as module variable
rg "^[[:space:]]*(allocatable|target).*models|type.*t_model_array.*models" src/Repository: MFlowCode/MFC Length of output: 146 🏁 Script executed: # Verify the coding guideline reference about pointer encapsulation
grep -A 5 "pointer.*encapsulation\|encapsulation.*pointer" .cursor/rules/mfc-agent-rules.mdcRepository: MFlowCode/MFC Length of output: 39 Add The 🛠️ Suggested fix type :: t_model_array
- type(t_model), allocatable :: model
- real(wp), allocatable, dimension(:, :, :) :: boundary_v
- real(wp), allocatable, dimension(:, :) :: interpolated_boundary_v
+ type(t_model), allocatable, target :: model
+ real(wp), allocatable, target, dimension(:, :, :) :: boundary_v
+ real(wp), allocatable, target, dimension(:, :) :: interpolated_boundary_v
integer :: boundary_edge_count
integer :: total_vertices
logical :: interpolate
end type t_model_array📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| !> Derived type adding initial condition (ic) patch parameters as attributes | ||||||||||||||||||||||||||||||||||
| !! NOTE: The requirements for the specification of the above parameters | ||||||||||||||||||||||||||||||||||
| !! are strongly dependent on both the choice of the multicomponent flow | ||||||||||||||||||||||||||||||||||
|
|
@@ -437,6 +447,8 @@ module m_derived_types | |||||||||||||||||||||||||||||||||
| integer, dimension(3) :: ip_grid !< Top left grid point of IP | ||||||||||||||||||||||||||||||||||
| real(wp), dimension(2, 2, 2) :: interp_coeffs !< Interpolation Coefficients of image point | ||||||||||||||||||||||||||||||||||
| integer :: ib_patch_id !< ID of the IB Patch the ghost point is part of | ||||||||||||||||||||||||||||||||||
| real(wp) :: levelset | ||||||||||||||||||||||||||||||||||
| real(wp), dimension(1:3) :: levelset_norm | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+450
to
+451
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. Suggestion: The newly added Severity Level: Critical 🚨- ❌ Ghost-point classification may be incorrect.
- ❌ Levelset-based IB forces/markers miscomputed.
- ⚠️ Simulation results nondeterministic across runs.
Suggested change
Steps of Reproduction ✅1. Inspect the ghost_point type in src/common/m_derived_types.fpp lines 448-455; lines
451-452 add levelset and levelset_norm fields.
2. Create a minimal reproducer (or find IB code that allocates ghost points) that declares
a ghost_point variable and reads the fields before any assignment:
- use m_derived_types
- type(ghost_point) :: g
- print *, g%levelset, g%levelset_norm
3. Compile and run the reproducer. The printed values will be indeterminate
(uninitialized), demonstrating that reading these fields without prior initialization
yields garbage or non-deterministic values.
4. In a real simulation run, any IB/ghost-point routine that reads g%levelset or
g%levelset_norm for classification or interpolation before explicitly writing them (for
example, ghost-point setup or levelset-based boundary condition code paths) will make
wrong decisions; verify by locating IB routines that use ghost_point fields after
allocation and before explicit assignment.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/common/m_derived_types.fpp
**Line:** 451:452
**Comment:**
*Possible Bug: The newly added `levelset` and `levelset_norm` fields in `ghost_point` are not initialized, so any code that reads these values before they are explicitly set (e.g., for classification or interpolation) will operate on garbage data and may misclassify ghost points.
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. |
||||||||||||||||||||||||||||||||||
| logical :: slip | ||||||||||||||||||||||||||||||||||
| integer, dimension(3) :: DB | ||||||||||||||||||||||||||||||||||
| end type ghost_point | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
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: The component
modelint_model_arrayis declared as a scalar allocatable rather than an allocatable array, which prevents this type from correctly representing multiple models; any calling code that expects to store or iterate over several models in this container will either silently only store one model or malfunction when trying to index it. [logic error]Severity Level: Major⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖