-
Notifications
You must be signed in to change notification settings - Fork 129
Analytic mibm velocities and airfoil centroid #1111
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 all commits
89a0ac5
ef446f5
2f9b1be
d54bc5b
697b5a0
6ed8350
7bd28cd
f40422a
541e727
cb7b845
54c9421
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,3 +6,8 @@ | |
| #:def analytical() | ||
|
|
||
| #:enddef | ||
|
|
||
| ! For moving immersed boundaries in simulation | ||
| #:def mib_analytical() | ||
|
|
||
| #:enddef | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -279,6 +279,7 @@ contains | |||||
| do i = 0, m | ||||||
| xy_local = [x_cc(i) - center(1), y_cc(j) - center(2), 0._wp] ! get coordinate frame centered on IB | ||||||
| xy_local = matmul(inverse_rotation, xy_local) ! rotate the frame into the IB's coordinates | ||||||
| xy_local = xy_local - patch_ib(patch_id)%centroid_offset ! airfoils are a patch that require a centroid offset | ||||||
|
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 centroid offset is subtracted from Severity Level: Critical 🚨- ❌ 2D airfoil placement in s_ib_airfoil mispositioned.
- ⚠️ Pitching-airfoil tests produce incorrect geometry mapping.
Suggested change
Steps of Reproduction ✅1. Prepare a 2D case that includes an airfoil patch (patch_ib(... )%geometry == 4). The 2D
branch in s_apply_ib_patches calls s_ib_airfoil (src/common/m_ib_patches.fpp — the 2D IB
patch logic that eventually invokes s_ib_airfoil).
2. Ensure the patch has a non-zero centroid_offset and a non-trivial rotation (set
patch_ib(patch_id)%centroid_offset and non-zero patch_ib(patch_id)%angles so
rotation_matrix_inverse != identity). Run to the phase where IB markers are populated so
s_ib_airfoil executes.
3. Inside s_ib_airfoil, execution computes xy_local and rotates it at the lines shown in
the diff (see src/common/m_ib_patches.fpp:279-281 where xy_local is computed and rotated,
then the subtraction at line 282). Because the code subtracts
patch_ib(patch_id)%centroid_offset without rotating it, the offset is applied in the
already-rotated/local frame incorrectly.
4. Observe incorrect IB marking: cell membership checks that follow (the if on xy_local(1)
at src/common/m_ib_patches.fpp:284 and subsequent assignments to ib_markers_sf) will mark
the airfoil in the wrong location or shape. This reproduces reliably when centroid_offset
is set and rotation is non-zero; the fix (rotate centroid_offset into local frame) aligns
coordinate bases and corrects marker placement.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/common/m_ib_patches.fpp
**Line:** 282:282
**Comment:**
*Logic Error: The centroid offset is subtracted from `xy_local` after rotating the global coordinate into the IB frame, but `patch_ib(patch_id)%centroid_offset` is likely defined in the global/model frame and must be rotated into the same frame before subtraction — otherwise the offset is applied in the wrong coordinate system causing misplaced airfoil geometry. Rotate the offset into the IB/local frame (or subtract the offset before rotation) so both vectors are in the same coordinate basis.
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 (xy_local(1) >= 0._wp .and. xy_local(1) <= ca_in) then | ||||||
| xa = xy_local(1)/ca_in | ||||||
|
|
@@ -433,6 +434,7 @@ contains | |||||
| do i = 0, m | ||||||
| xyz_local = [x_cc(i) - center(1), y_cc(j) - center(2), z_cc(l) - center(3)] ! get coordinate frame centered on IB | ||||||
| xyz_local = matmul(inverse_rotation, xyz_local) ! rotate the frame into the IB's coordinates | ||||||
| xyz_local = xyz_local - patch_ib(patch_id)%centroid_offset ! airfoils are a patch that require a centroid offset | ||||||
|
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 3D airfoil routine subtracts Severity Level: Critical 🚨- ❌ 3D airfoil marker placement incorrect in s_ib_3D_airfoil.
- ⚠️ 3D pitching/rotating airfoil simulations show wrong geometry.
Suggested change
Steps of Reproduction ✅1. Prepare a 3D case with an airfoil patch (patch_ib(... )%geometry == 11) so
s_apply_ib_patches calls s_ib_3D_airfoil during marker generation.
2. Set patch_ib(patch_id)%centroid_offset to a non-zero vector and set non-zero rotation
angles so rotation_matrix_inverse is non-trivial; run to the IB marker generation step.
3. In s_ib_3D_airfoil the code computes xyz_local and applies inverse_rotation at
src/common/m_ib_patches.fpp:435-436, then subtracts patch_ib(patch_id)%centroid_offset at
line 437 without rotating that offset into the local frame.
4. As a result the z/y/x cell-inclusion tests (for example the z_min/z_max check at the
surrounding block) and the later assignments to ib_markers_sf(i,j,l) are done with an
inconsistent offset and the 3D airfoil appears shifted. Reproduces when centroid_offset ≠
0 and rotation ≠ identity.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/common/m_ib_patches.fpp
**Line:** 437:437
**Comment:**
*Logic Error: The 3D airfoil routine subtracts `patch_ib(patch_id)%centroid_offset` from `xyz_local` after applying `inverse_rotation`; if `centroid_offset` is expressed in the global/model frame this subtraction is done in the wrong basis. Rotate the centroid offset into the IB/local frame with the same `inverse_rotation` before subtracting to ensure the offset is applied consistently in the same coordinate system.
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 (xyz_local(3) >= z_min .and. xyz_local(3) <= z_max) then | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -95,13 +95,15 @@ contains | |||||||||||||||||||
| integer :: i, j, k | ||||||||||||||||||||
| integer :: max_num_gps, max_num_inner_gps | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ! do all set up for moving immersed boundaries | ||||||||||||||||||||
| moving_immersed_boundary_flag = .false. | ||||||||||||||||||||
| do i = 1, num_ibs | ||||||||||||||||||||
| if (patch_ib(i)%moving_ibm /= 0) then | ||||||||||||||||||||
| call s_compute_moment_of_inertia(i, patch_ib(i)%angular_vel) | ||||||||||||||||||||
| moving_immersed_boundary_flag = .true. | ||||||||||||||||||||
| end if | ||||||||||||||||||||
| call s_update_ib_rotation_matrix(i) | ||||||||||||||||||||
| call s_compute_centroid_offset(i) | ||||||||||||||||||||
|
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: Performance / correctness: s_compute_centroid_offset is called for every IB inside the initial setup loop before IB markers are populated; this causes an expensive full-domain scan per IB and also runs before Severity Level: Critical 🚨- ❌ Startup initialization spends excessive CPU time.
- ⚠️ Centroid offsets computed from empty ib_markers.
- ❌ Ghost-point velocity uses wrong centroids.
- ⚠️ Pitching airfoil initial motion affected.
Suggested change
Steps of Reproduction ✅1. Start the code path that calls the IBM setup routine: the public subroutine s_ibm_setup
in src/simulation/m_ibm.fpp is executed (see s_ibm_setup loop at lines 98-108).
2. Inside s_ibm_setup (lines 98-108) the code iterates do i = 1, num_ibs and
unconditionally calls s_compute_centroid_offset(i) at line 106 before any IB marker
population steps.
3. s_compute_centroid_offset (defined at lines 1155-1207) scans ib_markers%sf over the
full domain to count cells (it depends on ib_markers contents). At this point ib_markers
was only allocated earlier (s_initialize_ibm_module) but not populated by
s_apply_ib_patches / s_populate_ib_buffers yet, so entries are effectively empty/zero.
4. Observable effects when reproducing: you will see s_compute_centroid_offset perform a
full-domain nested loop (i=0..m, j=0..n, k=0..p) for every IB (costly) and compute zero
cell counts / zero centroid results because ib_markers%sf contains no IB assignments yet.
This reproduces wasted work and incorrect centroid results on startup.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 106:106
**Comment:**
*Performance: Performance / correctness: s_compute_centroid_offset is called for every IB inside the initial setup loop before IB markers are populated; this causes an expensive full-domain scan per IB and also runs before `ib_markers%sf` is initialized/populated (leading to zero-cell counts). Remove the call from this early loop and compute centroid offsets later after IB patches/markers are applied and buffers populated.
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. |
||||||||||||||||||||
| end do | ||||||||||||||||||||
| $:GPU_ENTER_DATA(copyin='[patch_ib]') | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -1150,6 +1152,60 @@ contains | |||||||||||||||||||
|
|
||||||||||||||||||||
| end subroutine s_finalize_ibm_module | ||||||||||||||||||||
|
|
||||||||||||||||||||
| !> Computes the center of mass for IB patch types where we are unable to determine their center of mass analytically. | ||||||||||||||||||||
| !> These patches include things like NACA airfoils and STL models | ||||||||||||||||||||
| subroutine s_compute_centroid_offset(ib_marker) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| integer, intent(in) :: ib_marker | ||||||||||||||||||||
|
|
||||||||||||||||||||
| integer :: i, j, k, num_cells, num_cells_local | ||||||||||||||||||||
| real(wp), dimension(1:3) :: center_of_mass | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ! Offset only needs to be computes for specific geometries | ||||||||||||||||||||
| if (patch_ib(ib_marker)%geometry == 4 .or. & | ||||||||||||||||||||
| patch_ib(ib_marker)%geometry == 5 .or. & | ||||||||||||||||||||
| patch_ib(ib_marker)%geometry == 11 .or. & | ||||||||||||||||||||
| patch_ib(ib_marker)%geometry == 12) then | ||||||||||||||||||||
|
|
||||||||||||||||||||
| center_of_mass = [0._wp, 0._wp, 0._wp] | ||||||||||||||||||||
| num_cells_local = 0 | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ! get the summed mass distribution and number of cells to divide by | ||||||||||||||||||||
| do i = 0, m | ||||||||||||||||||||
| do j = 0, n | ||||||||||||||||||||
| do k = 0, p | ||||||||||||||||||||
| if (ib_markers%sf(i, j, k) == ib_marker) then | ||||||||||||||||||||
| num_cells_local = num_cells_local + 1 | ||||||||||||||||||||
| center_of_mass = center_of_mass + [x_cc(i), y_cc(j), 0._wp] | ||||||||||||||||||||
| if (num_dims == 3) center_of_mass(3) = center_of_mass(3) + z_cc(k) | ||||||||||||||||||||
| end if | ||||||||||||||||||||
| end do | ||||||||||||||||||||
| end do | ||||||||||||||||||||
| end do | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ! reduce the mass contribution over all MPI ranks and compute COM | ||||||||||||||||||||
| ! print *, "Before reduction ", center_of_mass, num_cells_local | ||||||||||||||||||||
| call s_mpi_allreduce_sum(center_of_mass(1), center_of_mass(1)) | ||||||||||||||||||||
| call s_mpi_allreduce_sum(center_of_mass(2), center_of_mass(2)) | ||||||||||||||||||||
| call s_mpi_allreduce_sum(center_of_mass(3), center_of_mass(3)) | ||||||||||||||||||||
| call s_mpi_allreduce_integer_sum(num_cells_local, num_cells) | ||||||||||||||||||||
| center_of_mass = center_of_mass/real(num_cells, wp) | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
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: Division-by-zero and incorrect averaging: the code reduces the summed coordinates and the integer cell count and unconditionally divides by Severity Level: Critical 🚨- ❌ Simulation startup can crash (division by zero).
- ❌ Pitching airfoil initialisation fails.
- ⚠️ Any IB with geometry types 4,5,11,12 affected.
- ⚠️ MPI job may produce NaNs across ranks.
Suggested change
Steps of Reproduction ✅1. Trigger the startup path that runs s_ibm_setup (src/simulation/m_ibm.fpp lines 98-108)
so that s_compute_centroid_offset is invoked (calls at line 106).
2. Inside s_compute_centroid_offset (subroutine at lines 1155-1207) the routine tallies
num_cells_local on each rank and then calls s_mpi_allreduce_integer_sum to produce
num_cells (see lines 1188-1192).
3. If no grid cell was assigned to this IB on any rank (num_cells == 0 after allreduce)
the code executes center_of_mass = center_of_mass/real(num_cells, wp) at line 1192,
producing a division-by-zero (or invalid NaN) at runtime.
4. Reproduce by running a minimal case where the IB geometry guarded by the if-condition
(geometry == 4,5,11,12) is present but ib_markers are empty (this occurs during current
setup ordering), observe crash/NaN at startup originating from s_compute_centroid_offset
lines 1188-1192.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/simulation/m_ibm.fpp
**Line:** 1193:1193
**Comment:**
*Possible Bug: Division-by-zero and incorrect averaging: the code reduces the summed coordinates and the integer cell count and unconditionally divides by `num_cells`. If no cells are found for the IB across all ranks this will divide by zero. Reduce the total cell-volume (or count) and check the global total before dividing; if zero, set a safe default and return.
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. |
||||||||||||||||||||
| ! assign the centroid offset as a vector pointing from the true COM to the "centroid" in the input file and replace the current centroid | ||||||||||||||||||||
| patch_ib(ib_marker)%centroid_offset = [patch_ib(ib_marker)%x_centroid, patch_ib(ib_marker)%y_centroid, patch_ib(ib_marker)%z_centroid] & | ||||||||||||||||||||
| - center_of_mass | ||||||||||||||||||||
| patch_ib(ib_marker)%x_centroid = center_of_mass(1) | ||||||||||||||||||||
| patch_ib(ib_marker)%y_centroid = center_of_mass(2) | ||||||||||||||||||||
| patch_ib(ib_marker)%z_centroid = center_of_mass(3) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ! rotate the centroid offset back into the local coords of the IB | ||||||||||||||||||||
| patch_ib(ib_marker)%centroid_offset = matmul(patch_ib(ib_marker)%rotation_matrix_inverse, patch_ib(ib_marker)%centroid_offset) | ||||||||||||||||||||
| else | ||||||||||||||||||||
| patch_ib(ib_marker)%centroid_offset(:) = [0._wp, 0._wp, 0._wp] | ||||||||||||||||||||
| end if | ||||||||||||||||||||
|
|
||||||||||||||||||||
| end subroutine s_compute_centroid_offset | ||||||||||||||||||||
|
|
||||||||||||||||||||
| subroutine s_compute_moment_of_inertia(ib_marker, axis) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| real(wp), dimension(3), intent(in) :: axis !< the axis about which we compute the moment. Only required in 3D. | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
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 toolchain code for analytic IB motion incorrectly allows spatial variables ( Solution Walkthrough:Before:# toolchain/mfc/case.py
def __get_analytic_mib_fpp(self, print: bool) -> str:
# ...
def rhs_replace(match):
return {
'x': 'x_cc(i)', 'y': 'y_cc(j)', 'z': 'z_cc(k)',
't': 'mytime',
'r': f'patch_ib({pid})%radius',
'e' : f'{math.e}',
}.get(match.group(), match.group())
# ...
for attribute, expr in items:
rhs = re.sub(r"[a-zA-Z]+", rhs_replace, expr)
lines.append(f" {attribute} = {rhs}")
# ...After:# toolchain/mfc/case.py
def __get_analytic_mib_fpp(self, print: bool) -> str:
# ...
def rhs_replace(match):
return {
't': 'mytime',
'r': f'patch_ib({pid})%radius',
'e' : f'{math.e}',
}.get(match.group(), match.group())
# ...
for attribute, expr in items:
rhs = re.sub(r"[a-zA-Z]+", rhs_replace, expr)
lines.append(f" {attribute} = {rhs}")
# ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,12 +7,15 @@ | |
| from .state import ARG | ||
| from .run import case_dicts | ||
|
|
||
|
|
||
| QPVF_IDX_VARS = { | ||
| 'alpha_rho': 'contxb', 'vel' : 'momxb', 'pres': 'E_idx', | ||
| 'alpha': 'advxb', 'tau_e': 'stress_idx%beg', 'Y': 'chemxb', | ||
| 'cf_val': 'c_idx', 'Bx': 'B_idx%beg', 'By': 'B_idx%end-1', 'Bz': 'B_idx%end', | ||
| } | ||
|
|
||
| MIBM_ANALYTIC_VARS = [ | ||
| 'vel(1)', 'vel(2)', 'vel(3)', 'angular_vel(1)', 'angular_vel(2)', 'angular_vel(3)' | ||
| ] | ||
| # "B_idx%end - 1" not "B_idx%beg + 1" must be used because 1D does not have Bx | ||
|
|
||
| @dataclasses.dataclass(init=False) | ||
|
|
@@ -48,7 +51,7 @@ def get_inp(self, _target) -> str: | |
| dict_str = "" | ||
| for key, val in self.params.items(): | ||
| if key in MASTER_KEYS and key not in case_dicts.IGNORE: | ||
| if self.__is_ic_analytical(key, val): | ||
| if self.__is_ic_analytical(key, val) or self.__is_mib_analytical(key, val): | ||
| dict_str += f"{key} = 0d0\n" | ||
| ignored.append(key) | ||
| continue | ||
|
|
@@ -95,8 +98,20 @@ def __is_ic_analytical(self, key: str, val: str) -> bool: | |
|
|
||
| return False | ||
|
|
||
| def __is_mib_analytical(self, key: str, val: str) -> bool: | ||
| '''Is this initial condition analytical? | ||
| More precisely, is this an arbitrary expression or a string representing a number?''' | ||
| if common.is_number(val) or not isinstance(val, str): | ||
| return False | ||
|
|
||
| for variable in MIBM_ANALYTIC_VARS: | ||
| if re.match(fr'^patch_ib\([0-9]+\)%{re.escape(variable)}', key): | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
| # pylint: disable=too-many-locals | ||
| def __get_pre_fpp(self, print: bool) -> str: | ||
| def __get_analytic_ic_fpp(self, print: bool) -> str: | ||
| # generates the content of an FFP file that will hold the functions for | ||
| # some initial condition | ||
| DATA = { | ||
|
|
@@ -181,6 +196,72 @@ def rhs_replace(match): | |
| #:def analytical() | ||
| {f'{chr(10)}{chr(10)}'.join(srcs)} | ||
| #:enddef | ||
| """ | ||
| return content | ||
|
|
||
| # gets the analytic description of a moving IB's velocity and rotation rate | ||
| def __get_analytic_mib_fpp(self, print: bool) -> str: | ||
| # iterates over the parameters and checks if they are defined as an | ||
| # analytical function. If so, append it to the `patches`` object | ||
| ib_patches = {} | ||
|
|
||
| for key, val in self.params.items(): | ||
| if not self.__is_mib_analytical(key, val): | ||
| continue | ||
|
|
||
| patch_id = re.search(r'[0-9]+', key).group(0) | ||
|
|
||
| if patch_id not in ib_patches: | ||
| ib_patches[patch_id] = [] | ||
|
|
||
| ib_patches[patch_id].append((key, val)) | ||
|
|
||
| srcs = [] | ||
|
|
||
| # for each analytical patch that is required to be added, generate | ||
| # the string that contains that function. | ||
| for pid, items in ib_patches.items(): | ||
|
|
||
| # function that defines how we will replace variable names with | ||
| # values from the case file | ||
| def rhs_replace(match): | ||
|
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. P1: Analytic IB codegen replaces Prompt for AI agents |
||
| return { | ||
| 'x': 'x_cc(i)', 'y': 'y_cc(j)', 'z': 'z_cc(k)', | ||
| 't': 'mytime', | ||
|
|
||
| 'r': f'patch_ib({pid})%radius', | ||
|
|
||
| 'e' : f'{math.e}', | ||
| }.get(match.group(), match.group()) | ||
|
|
||
| lines = [] | ||
| # perform the replacement of strings for each analytic function | ||
| # to generate some fortran string representing the code passed in | ||
| for attribute, expr in items: | ||
| if print: | ||
| cons.print(f"* Codegen: {attribute} = {expr}") | ||
|
|
||
| lhs = attribute | ||
| rhs = re.sub(r"[a-zA-Z]+", rhs_replace, expr) | ||
|
|
||
| lines.append(f" {lhs} = {rhs}") | ||
|
|
||
| # concatenates all of the analytic lines into a single string with | ||
| # each element separated by new line characters. Then write those | ||
| # new lines as a fully concatenated string with fortran syntax | ||
| srcs.append(f"""\ | ||
| if (i == {pid}) then | ||
| {f'{chr(10)}'.join(lines)} | ||
| end if\ | ||
| """) | ||
|
|
||
| content = f"""\ | ||
| ! This file was generated by MFC. It is only used when we analytically | ||
| ! parameterize the velocity and rotation rate of a moving IB. | ||
|
|
||
| #:def mib_analytical() | ||
| {f'{chr(10)}{chr(10)}'.join(srcs)} | ||
| #:enddef | ||
| """ | ||
| return content | ||
|
|
||
|
|
@@ -266,9 +347,16 @@ def __get_sim_fpp(self, print: bool) -> str: | |
| else: | ||
| out = "" | ||
|
|
||
| out = out + self.__get_analytic_mib_fpp(print) | ||
|
|
||
| # We need to also include the pre_processing includes so that common subroutines have access to the @:analytical function | ||
| return out + f"\n{self.__get_pre_fpp(print)}" | ||
|
|
||
|
|
||
| def __get_pre_fpp(self, print: bool) -> str: | ||
| out = self.__get_analytic_ic_fpp(print) | ||
| return out | ||
|
|
||
| def get_fpp(self, target, print = True) -> str: | ||
| def _prepend() -> str: | ||
| return f"""\ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -121,9 +121,7 @@ def analytic(self): | |||||||||||||||||
| PRE_PROCESS[f"patch_ib({ib_id})%{real_attr}"] = ty | ||||||||||||||||||
|
|
||||||||||||||||||
| for dir_id in range(1, 4): | ||||||||||||||||||
| PRE_PROCESS[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL | ||||||||||||||||||
| PRE_PROCESS[f"patch_ib({ib_id})%angles({dir_id})"] = ParamType.REAL | ||||||||||||||||||
| PRE_PROCESS[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL | ||||||||||||||||||
|
|
||||||||||||||||||
| for cmp_id, cmp in enumerate(["x", "y", "z"]): | ||||||||||||||||||
| cmp_id += 1 | ||||||||||||||||||
|
|
@@ -372,9 +370,9 @@ def analytic(self): | |||||||||||||||||
| SIMULATION[f"patch_ib({ib_id})%{real_attr}"] = ty | ||||||||||||||||||
|
|
||||||||||||||||||
| for dir_id in range(1, 4): | ||||||||||||||||||
| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL | ||||||||||||||||||
| SIMULATION[f"patch_ib({ib_id})%vel({dir_id})"] = ParamType.REAL.analytic() | ||||||||||||||||||
|
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. P1: Assigning Prompt for AI agents
Suggested change
|
||||||||||||||||||
| SIMULATION[f"patch_ib({ib_id})%angles({dir_id})"] = ParamType.REAL | ||||||||||||||||||
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL | ||||||||||||||||||
| SIMULATION[f"patch_ib({ib_id})%angular_vel({dir_id})"] = ParamType.REAL.analytic() | ||||||||||||||||||
|
Comment on lines
+373
to
+375
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: Type mismatch: assigning the result of Severity Level: Critical 🚨- ❌ Module import fails building schema and validator.
- ❌ get_validator() unusable; schema compilation blocked.
- ⚠️ Any tool consuming CASE options fails initialization.
Suggested change
Steps of Reproduction ✅1. Open the module file toolchain/mfc/run/case_dicts.py and locate the
ParamType.analytic() implementation (function at toolchain/mfc/run/case_dicts.py:17-22).
The analytic() returns a plain dict for ParamType.REAL.
2. Observe the SIMULATION assignments inside the ib loop at the hunk lines where the PR
added analytic variants (see toolchain/mfc/run/case_dicts.py lines 369 and 371): those two
lines assign the raw dicts returned from ParamType.REAL.analytic() into SIMULATION.
3. Import the module (e.g., python -c "import toolchain.mfc.run.case_dicts as c") or call
get_validator() from toolchain/mfc/run/case_dicts.py. Module import triggers building of
ALL and then the comprehension _properties = { k: v.value for k, v in ALL.items() } (the
_properties line is defined immediately after ALL is assembled in the same file).
4. At that comprehension, Python evaluates v.value for every value in ALL. For the two
entries assigned above, v is a plain dict and has no .value attribute; this raises
AttributeError: 'dict' object has no attribute 'value'. The import or get_validator() call
fails with that exception.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** toolchain/mfc/run/case_dicts.py
**Line:** 369:371
**Comment:**
*Type Error: Type mismatch: assigning the result of `ParamType.REAL.analytic()` (a plain dict) directly into `SIMULATION` mixes raw dicts with `ParamType` Enum members. Later the code builds `_properties = {k: v.value for k, v in ALL.items()}` which assumes every value is an Enum with a `.value` attribute; a raw dict will raise AttributeError. Replace the two analytic assignments with the Enum member `ParamType.REAL` to keep dictionary values consistent and avoid the runtime AttributeError.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
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. P1: Same issue as with Prompt for AI agents
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| for cmp_id, cmp in enumerate(["x", "y", "z"]): | ||||||||||||||||||
| cmp_id += 1 | ||||||||||||||||||
|
|
||||||||||||||||||
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: Coordinate-frame mixup:
centroid_offsetis subtracted after applyinginverse_rotation, which treats the offset as if it were expressed in the rotated/local frame; ifcentroid_offsetis defined in the global frame (ascenteris), this produces incorrect local coordinates and wrong nearest-point logic. Subtract thecentroid_offsetbefore rotation so the subtraction happens in the same (global) coordinate frame as the initial subtraction fromcenter. [logic error]Severity Level: Critical 🚨
Steps of Reproduction ✅
Prompt for AI Agent 🤖