Fix/Add Patches 13 (2D modal) and 14 (3D spherical harmonics)#1163
Fix/Add Patches 13 (2D modal) and 14 (3D spherical harmonics)#1163ChrisZYJ wants to merge 12 commits intoMFlowCode:masterfrom
Conversation
codeant-ai
bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/1967AE06/golden-metadata.txt (1)
7-7: Golden metadata generated from a dirty working tree.The git state shows
(dirty), meaning there were uncommitted changes when this golden file was generated. Consider regenerating the golden files from a clean commit to ensure the test baselines are reproducible and traceable. Based on learnings: "Run./mfc.sh testlocally before requesting review" and "Use./mfc.sh test --generate -o <test_id>to create golden files for new test cases."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/1967AE06/golden-metadata.txt` at line 7, The golden metadata shows a "(dirty)" git state which means it was generated with uncommitted changes; to fix, reset or commit your working tree so it's clean, regenerate the golden file for the test ID 1967AE06 by running the referenced test-generation command (e.g. run ./mfc.sh test --generate -o 1967AE06 or run ./mfc.sh test locally) and replace tests/1967AE06/golden-metadata.txt with the new output so the Git line no longer contains "(dirty)" and the baseline is reproducible.examples/2D_patch_modal_shape_exp/case.py (1)
7-7: Nit: DefineLyexplicitly for clarity.
Lxis used for bothlength_xandlength_y(line 74). The additive-form example defines bothLx, Ly. For consistency and readability, consider definingLy = 8.0explicitly here too.Proposed fix
-Lx = 8.0 +Lx, Ly = 8.0, 8.0and
- "patch_icpp(1)%length_y": Lx, + "patch_icpp(1)%length_y": Ly,Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/2D_patch_modal_shape_exp/case.py` at line 7, Define Ly explicitly to match Lx instead of reusing Lx for both dimensions: add a line setting Ly = 8.0 and update any places that set length_y (or pass length_y) to use Ly (e.g., where length_x and length_y are supplied) so the mesh/patch geometry uses Lx and Ly separately; update the example to mirror the additive-form pattern.src/common/m_derived_types.fpp (1)
219-232: Verify all three executables after extendingic_patch_parameters.Since this type lives in
src/common, please confirm pre_process, simulation, and post_process builds/tests still pass with the new fields.Based on learnings: "Code in
src/common/affects all three executables (pre_process, simulation, post_process). Changes here have wide blast radius and require testing all three targets."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/m_derived_types.fpp` around lines 219 - 232, You extended the derived type used across executables (see ic_patch_parameters in m_derived_types.fpp and nearby fields like fourier_cos, fourier_sin, modal_clip_r_to_min, modal_r_min, modal_use_exp_form, sph_har_coeff), so rebuild and run all three targets: pre_process, simulation, and post_process; fix any compile/link errors by updating all modules and call sites that reference ic_patch_parameters (parsing, I/O/serialization, initial-condition setup), update any input/config files or unit tests that instantiate this type, and run the test suites for each executable to confirm no runtime failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/documentation/case.md`:
- Around line 1105-1117: The inline LaTeX math in the Geometry 13 documentation
uses $...$ which fails the linter; update every inline occurrence to use the
sphinx-friendly \f$...\f$ form (e.g., replace θ = atan2(...) written as $...$
with \f$...\f$ and similarly for R_boundary, radius, fourier_cos, fourier_sin,
modal_use_exp_form, modal_clip_r_to_min, modal_r_min expressions); do not change
the math content, only swap the inline delimiters throughout that paragraph so
the formulas remain identical but use \f$...\f$.
In `@src/pre_process/m_icpp_patches.fpp`:
- Around line 1042-1152: Both s_icpp_2d_modal and s_icpp_3d_spherical_harmonic
are missing the analytic/hardcoded override hooks used by other patch routines;
add the same call sequence used elsewhere so analytic expressions or hardcoded
patches can override before falling back to s_assign_patch_primitive_variables.
Specifically, at the site where you currently call
s_assign_patch_primitive_variables (inside the if that checks r<=R and
alter_patch/smooth_patch_id), first invoke the project's analytical override
hook and the hardcoded override hook (the same functions/calls used by other
s_icpp_* routines), and only call s_assign_patch_primitive_variables when those
hooks did not perform an override; make the change in both s_icpp_2d_modal and
s_icpp_3d_spherical_harmonic.
In `@tests/87FBC893/golden-metadata.txt`:
- Around line 5-7: The golden-metadata.txt files contain machine-specific
absolute paths and git state and should not be tracked; update .gitignore to
stop forcing these files into the repo by removing or changing the negated
pattern `!/tests/*/golden-metadata.txt` (or add an explicit ignore entry for
tests/*/golden-metadata.txt), then untrack existing committed files (e.g., git
rm --cached tests/*/golden-metadata.txt) and commit the change so future runs
don’t produce spurious diffs; ensure any CI that relied on these files is
adjusted to regenerate them as needed.
---
Duplicate comments:
In `@tests/3F864CFF/golden-metadata.txt`:
- Around line 5-7: The golden-metadata file contains machine-specific and
working-tree state data (lines starting with "Invocation:", "Lock:", and "Git:")
that makes tests non-deterministic; update the metadata generation so it does
not embed local invocation paths or the dirty branch state: remove or normalize
the "Invocation:" path, remove machine-specific "Lock:" details or replace with
stable feature flags, and ensure "Git:" records a stable commit hash only (no
branch name or "dirty" marker) or use a placeholder when the repo isn't clean.
Locate the code that writes golden-metadata.txt (search for writers emitting the
"Invocation:", "Lock:", or "Git:" strings) and change it to emit
normalized/placeholder values or omit these lines so the golden file is
machine-independent.
---
Nitpick comments:
In `@examples/2D_patch_modal_shape_exp/case.py`:
- Line 7: Define Ly explicitly to match Lx instead of reusing Lx for both
dimensions: add a line setting Ly = 8.0 and update any places that set length_y
(or pass length_y) to use Ly (e.g., where length_x and length_y are supplied) so
the mesh/patch geometry uses Lx and Ly separately; update the example to mirror
the additive-form pattern.
In `@src/common/m_derived_types.fpp`:
- Around line 219-232: You extended the derived type used across executables
(see ic_patch_parameters in m_derived_types.fpp and nearby fields like
fourier_cos, fourier_sin, modal_clip_r_to_min, modal_r_min, modal_use_exp_form,
sph_har_coeff), so rebuild and run all three targets: pre_process, simulation,
and post_process; fix any compile/link errors by updating all modules and call
sites that reference ic_patch_parameters (parsing, I/O/serialization,
initial-condition setup), update any input/config files or unit tests that
instantiate this type, and run the test suites for each executable to confirm no
runtime failures.
In `@tests/1967AE06/golden-metadata.txt`:
- Line 7: The golden metadata shows a "(dirty)" git state which means it was
generated with uncommitted changes; to fix, reset or commit your working tree so
it's clean, regenerate the golden file for the test ID 1967AE06 by running the
referenced test-generation command (e.g. run ./mfc.sh test --generate -o
1967AE06 or run ./mfc.sh test locally) and replace
tests/1967AE06/golden-metadata.txt with the new output so the Git line no longer
contains "(dirty)" and the baseline is reproducible.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/documentation/case.md`:
- Around line 1105-1117: The docs lack a Geometry 14 description for 3D
spherical-harmonic surfaces; add a block matching Geometry 13 that explains
coordinate conventions and coefficient semantics: describe that Geometry 14 (3D
spherical harmonic) defines the surface as r(θ,φ)=radius + sum_{l,m}
sph_har_coeff(l,m) * Y_l^m(θ,φ) (or an exponential variant if you support
modal_use_exp_form), state the polar and azimuthal conventions θ = acos(z/r) and
φ = atan2(y,x) relative to the centroid, clarify that the implementation uses a
real-Ylm basis and list the coefficient indexing (l = 0..L, m = -l..l) and
whether coefficients are absolute (length) or relative (dimensionless) and any
clipping behavior analogous to modal_clip_r_to_min/modal_r_min.
---
Duplicate comments:
In `@src/pre_process/m_icpp_patches.fpp`:
- Around line 1146-1156: The s_icpp_3d_spherical_harmonic subroutine is missing
the same hooks/bookkeeping/Fypp macros and local declarations present in other
3D patch routines; add the same variable declarations (analytical flags,
Hardcoded3D markers, bookkeeping arrays/indices and any Fypp-generated locals)
near the existing declarations around where s_icpp_3d_spherical_harmonic begins
(matching the pattern used in s_icpp_2d_modal and other 3D routines), insert the
analytical/@:Hardcoded3D/Fypp macro calls and bookkeeping statements immediately
after the call to s_assign_patch_primitive_variables(patch_id, i, j, k,
eta_local, q_prim_vf, patch_id_fp) to mirror other patches, and ensure any
allocated bookkeeping arrays are deallocated before the end of the subroutine to
match the resource handling in the other 3D patch implementations.
- Around line 1086-1095: In s_icpp_2d_modal, add the missing
analytical/hardcoded hooks, bookkeeping update, and Fypp variable/deallocation
macros: declare the
HardcodedDimensionsExtrusion()/Hardcoded2DVariables()/HardcodedDellacation()
variables near the existing declarations at the top of the subroutine, after the
declaration block around line ~1056; immediately after the call to
s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf,
patch_id_fp) insert calls to @:analytical() and an if-block for @:Hardcoded2D()
and update the patch_id_fp bookkeeping (set patch_id_fp(i,j,0)=patch_id where
appropriate); finally add the Fypp deallocation macro invocation
(HardcodedDellacation()) before the end of subroutine s_icpp_2d_modal so the
hardcoded 2D variables are cleaned up.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1163 +/- ##
==========================================
+ Coverage 44.95% 45.34% +0.39%
==========================================
Files 70 70
Lines 20504 20510 +6
Branches 1946 1951 +5
==========================================
+ Hits 9217 9301 +84
+ Misses 10166 10082 -84
- Partials 1121 1127 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sbryngelson I think this PR is ready. Could you please take a look? Thank you. |
|
i need to force it to run benchmark for now, since i somehow partially broke that and also we now have many amdflang workarounds for case optimization (used in benchmark) |
codeant-ai
bot
added
size:XXL
codeant-ai
bot
added
size:XXL
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pre_process/m_check_patches.fpp (1)
253-277:⚠️ Potential issue | 🟠 MajorAdd validation checks in
toolchain/mfc/case_validator.pyfor geometry 13/14 modal and spherical harmonic parameters.The new parameters
fourier_cos/sin(1-10),modal_clip_r_to_min,modal_r_min,modal_use_exp_form, and spherical harmonic coefficients are registered indefinitions.pybut lack corresponding validation in the case validator. Per project standards, add geometry-type-specific checks (e.g.,check_patch_icpp_geometry_modal_andcheck_patch_icpp_geometry_spherical_harmonic_) to validate:
- Fourier coefficient indexing and value ranges
- Modal option consistency (clip_r_to_min, r_min, use_exp_form constraints)
- Spherical harmonic coefficient ranges and completeness
Also document these validations in the
PHYSICS_DOCSdict as required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pre_process/m_check_patches.fpp` around lines 253 - 277, Add geometry-type-specific validation to toolchain/mfc/case_validator.py: implement check_patch_icpp_geometry_modal_ (called for geometry types 13/14 modal) to verify Fourier coefficients fourier_cos1..fourier_cos10 and fourier_sin1..fourier_sin10 are only indexed 1..10, numeric and within allowed ranges, and to enforce modal option consistency: if modal_clip_r_to_min is true then modal_r_min must be present and >0, modal_r_min must be >=0 when set, and modal_use_exp_form must be a valid boolean flag (and incompatible combinations flagged). Implement check_patch_icpp_geometry_spherical_harmonic_ to ensure spherical harmonic coefficient sets are complete for required orders, each coefficient is numeric and within valid ranges, and required centroids/radius checks are present. Register these new checks where other patch_icpp geometry checks are invoked and add concise entries describing the modal and spherical harmonic validations to the PHYSICS_DOCS dict.
🧹 Nitpick comments (2)
src/common/m_constants.fpp (1)
18-30: Run pre_process, simulation, and post_process tests for this common change.
These constants live insrc/common/, so a full three-target test sweep is prudent.Based on learnings: Code in
src/common/affects all three executables (pre_process, simulation, post_process) - changes have wide blast radius and must be tested thoroughly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/m_constants.fpp` around lines 18 - 30, Changes to common constants in src/common/m_constants.fpp (e.g., small_radius, num_fluids_max, num_probes_max, num_patches_max, num_bc_patches_max, max_2d_fourier_modes, max_sph_harm_degree) can impact all three executables; run the full test sweep: build and run the pre_process, simulation, and post_process targets (or their respective test suites) and verify no regressions or failures, paying special attention to code paths that read these parameters (stencil logic, Fourier/modal routines, spherical-harmonic geometry 14, and fluid/probe/patch allocation) and update any dependent tests or bounds if failures surface.src/common/m_helper.fpp (1)
569-572:m_order <= 0is alwaysm_order == 0after the domain guardAfter the early-return check on line 564 (
m_order < 0→ return), the remaining code hasm_order >= 0guaranteed. The conditionsm_order <= 0on lines 569 and 571 are therefore equivalent tom_order == 0, which is clearer and reflects the actual intent.♻️ Optional readability cleanup
- if (m_order <= 0 .and. l <= 0) then + if (m_order == 0 .and. l == 0) then result_P = 1._wp - elseif (l == 1 .and. m_order <= 0) then + elseif (l == 1 .and. m_order == 0) then result_P = x🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/m_helper.fpp` around lines 569 - 572, The conditions using "m_order <= 0" are misleading because earlier code already guards out negative m_order, so update the checks in this routine (referencing m_order and result_P in src/common/m_helper.fpp) to use "m_order == 0" instead of "<= 0" (apply to both occurrences that set result_P = 1._wp and result_P = x) to reflect the actual domain and intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/m_helper.fpp`:
- Line 538: The current prefac computation in real_ylm uses factorial(...) which
returns int64 and overflows for l+|m|≥21; replace the factorial-based ratio with
an overflow-safe log-gamma formulation: use the Fortran intrinsic log_gamma to
compute the log of factorials (log_gamma(n+1) == log(n!)) and compute the ratio
via exp(log_gamma(l - m_abs + 1) - log_gamma(l + m_abs + 1)) inside the sqrt,
keeping the rest of the expression (2*l+1)/(4._wp*pi) unchanged; update the
prefac assignment (the prefac symbol inside real_ylm) to use this log-gamma/exp
form so normalization is correct for all l and m.
- Line 579: The expression computing result_P uses double_factorial(2*l - 1)
which returns int64 and overflows for l≥18; replace the large-integer factorial
path with an overflow-free computation: compute P_l^l without calling
double_factorial by either (a) using the same real-type accumulation approach
used in real_ylm (convert multipliers to real(wp) and multiply progressively to
form the diagonal term) or (b) iteratively accumulate the diagonal recurrence
starting from P_0^0 = 1 and applying the diagonal step l times to produce
result_P; update the code that sets result_P to use one of these overflow-safe
strategies instead of real(double_factorial(...), wp).
In `@src/pre_process/m_check_patches.fpp`:
- Around line 253-264: In s_check_2d_modal_patch_geometry add validation for
patch_icpp(patch_id)%modal_r_min when patch_icpp(patch_id)%modal_clip_r_to_min
is true: check that modal_r_min is set (not f_is_default) and greater than zero
(and optionally less than or equal to patch_icpp(patch_id)%radius) by adding
appropriate @:PROHIBIT checks that reference
patch_icpp(patch_id)%modal_clip_r_to_min and patch_icpp(patch_id)%modal_r_min to
fail fast when clipping is enabled but the minimum radius is unset or
non-positive.
In `@src/pre_process/m_icpp_patches.fpp`:
- Around line 1153-1156: The code currently calls
s_assign_patch_primitive_variables for 3D spherical-harmonic patches but does
not update the bookkeeping array patch_id_fp, which breaks later alter/smoothing
logic; after the call to s_assign_patch_primitive_variables (inside the same
conditional that checks patch_icpp(patch_id)%alter_patch and smooth_patch_id)
set patch_id_fp(i, j, k) to patch_id so the footprint reflects the new
assignment, ensuring subsequent tests against patch_id_fp, smooth_patch_id, and
alter_patch behave correctly.
- Around line 1093-1096: After calling s_assign_patch_primitive_variables for 2D
modal patches you must update the patch bookkeeping so subsequent checks (e.g.
patch_icpp(...)%alter_patch(...) and smoothing using smooth_patch_id) see the
new owner; modify the branch that currently calls
s_assign_patch_primitive_variables(patch_id, i, j, 0, ...) to also set
patch_id_fp(i, j, 0) = patch_id immediately after the call so patch_id_fp
reflects the assignment.
---
Outside diff comments:
In `@src/pre_process/m_check_patches.fpp`:
- Around line 253-277: Add geometry-type-specific validation to
toolchain/mfc/case_validator.py: implement check_patch_icpp_geometry_modal_
(called for geometry types 13/14 modal) to verify Fourier coefficients
fourier_cos1..fourier_cos10 and fourier_sin1..fourier_sin10 are only indexed
1..10, numeric and within allowed ranges, and to enforce modal option
consistency: if modal_clip_r_to_min is true then modal_r_min must be present and
>0, modal_r_min must be >=0 when set, and modal_use_exp_form must be a valid
boolean flag (and incompatible combinations flagged). Implement
check_patch_icpp_geometry_spherical_harmonic_ to ensure spherical harmonic
coefficient sets are complete for required orders, each coefficient is numeric
and within valid ranges, and required centroids/radius checks are present.
Register these new checks where other patch_icpp geometry checks are invoked and
add concise entries describing the modal and spherical harmonic validations to
the PHYSICS_DOCS dict.
---
Duplicate comments:
In `@docs/documentation/case.md`:
- Around line 1105-1117: Add a full descriptive block for "Geometry 14"
mirroring the level of detail in "Geometry 13": define the coordinate convention
(how θ is computed, e.g., θ = atan2(y - y_centroid, x - x_centroid)), specify
the basis and functional form(s) (additive vs exponential controlled by
modal_use_exp_form), show the exact formulas for R_boundary using the same
coefficient naming scheme (e.g., fourier_cos(n), fourier_sin(n) or equivalent),
describe coefficient indexing and units (absolute vs relative), and document
clipping behavior and knobs (modal_clip_r_to_min, modal_r_min, clipping negative
R_boundary to zero) so readers can unambiguously reproduce and interpret
Geometry 14.
In `@src/pre_process/m_icpp_patches.fpp`:
- Around line 1093-1096: The block that calls s_assign_patch_primitive_variables
for patch faces (using patch_id_fp, patch_icpp%alter_patch, smooth_patch_id)
lacks the analytical/hardcoded geometry overrides for geometry types 13 and 14;
update this conditional branch to call the same analytical/hardcoded initializer
hooks used by other patch initializers when patch_icpp(patch_id)%geometry equals
13 or 14 (and/or when patch_id_fp indicates those geometries), ensuring the code
invokes the hardcoded/analytical override paths before or instead of
s_assign_patch_primitive_variables for these geometry IDs so the analytical
hooks for geometry 13/14 are executed consistently.
---
Nitpick comments:
In `@src/common/m_constants.fpp`:
- Around line 18-30: Changes to common constants in src/common/m_constants.fpp
(e.g., small_radius, num_fluids_max, num_probes_max, num_patches_max,
num_bc_patches_max, max_2d_fourier_modes, max_sph_harm_degree) can impact all
three executables; run the full test sweep: build and run the pre_process,
simulation, and post_process targets (or their respective test suites) and
verify no regressions or failures, paying special attention to code paths that
read these parameters (stencil logic, Fourier/modal routines, spherical-harmonic
geometry 14, and fluid/probe/patch allocation) and update any dependent tests or
bounds if failures surface.
In `@src/common/m_helper.fpp`:
- Around line 569-572: The conditions using "m_order <= 0" are misleading
because earlier code already guards out negative m_order, so update the checks
in this routine (referencing m_order and result_P in src/common/m_helper.fpp) to
use "m_order == 0" instead of "<= 0" (apply to both occurrences that set
result_P = 1._wp and result_P = x) to reflect the actual domain and intent.
| elseif (m_order > 0) then | ||
| Y = (-1._wp)**m_order*sqrt(2._wp)*prefactor*associated_legendre(x, l, m_order)*cos(m_order*phi); | ||
| x = cos(theta) | ||
| prefac = sqrt((2*l + 1)*real(factorial(l - m_abs), wp)/real(factorial(l + m_abs), wp)/(4._wp*pi)) |
There was a problem hiding this comment.
factorial(l + m_abs) silently overflows int64 for l + |m| ≥ 21
factorial returns an int64 result. 20! ≈ 2.43 × 10¹⁸ fits (barely); 21! ≈ 5.11 × 10¹⁹ exceeds int64 max (≈ 9.22 × 10¹⁸), so the integer wraps silently before the real(…, wp) cast, producing a wrong normalization coefficient.
The PR currently restricts to modes 1–10 (max l + |m| = 20, right at the boundary), but real_ylm is a public API function and contains no guard that enforces this limit. A user supplying l = 11 would get silently wrong Y_lm values.
A numerically safer and overflow-free alternative is to compute the log-ratio directly:
🛡️ Suggested fix: log-gamma ratio for normalization
- prefac = sqrt((2*l + 1)*real(factorial(l - m_abs), wp)/real(factorial(l + m_abs), wp)/(4._wp*pi))
+ ! Use log-gamma to avoid int64 overflow for large l
+ prefac = sqrt(real(2*l + 1, wp) &
+ *exp(log_gamma(real(l - m_abs + 1, wp)) - log_gamma(real(l + m_abs + 1, wp))) &
+ /(4._wp*pi))log_gamma is a Fortran 2008 intrinsic (standard on all MFC-targeted compilers) and handles arbitrary l without overflow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/m_helper.fpp` at line 538, The current prefac computation in
real_ylm uses factorial(...) which returns int64 and overflows for l+|m|≥21;
replace the factorial-based ratio with an overflow-safe log-gamma formulation:
use the Fortran intrinsic log_gamma to compute the log of factorials
(log_gamma(n+1) == log(n!)) and compute the ratio via exp(log_gamma(l - m_abs +
1) - log_gamma(l + m_abs + 1)) inside the sqrt, keeping the rest of the
expression (2*l+1)/(4._wp*pi) unchanged; update the prefac assignment (the
prefac symbol inside real_ylm) to use this log-gamma/exp form so normalization
is correct for all l and m.
| result_P = (-1)**l*double_factorial(2*l - 1)*(1 - x**2)**(l/2); | ||
| ! P_l^l(x) = (-1)^l (2l-1)!! (1-x^2)^(l/2). Use real exponent for odd l | ||
| one_minus_x2 = max(0._wp, 1._wp - x**2) | ||
| result_P = (-1)**l*real(double_factorial(2*l - 1), wp)*one_minus_x2**(0.5_wp*real(l, wp)) |
There was a problem hiding this comment.
double_factorial(2*l - 1) overflows int64 for l ≥ 18
double_factorial returns int64. 33!! ≈ 6.33 × 10¹⁸ fits; 35!! ≈ 2.22 × 10²⁰ overflows (l = 18, since 2×18−1 = 35). The subsequent real(…, wp) cast then silently returns a garbage P_l^l value.
For the current PR's l ≤ 10 use case this does not trigger (19!! ≈ 6.5 × 10⁸), but there is no guard inside the function. Consistent with the fix suggested for real_ylm, compute the diagonal term without large intermediate integers:
🛡️ Suggested fix for P_l^l
elseif (m_order == l) then
- ! P_l^l(x) = (-1)^l (2l-1)!! (1-x^2)^(l/2). Use real exponent for odd l
one_minus_x2 = max(0._wp, 1._wp - x**2)
- result_P = (-1)**l*real(double_factorial(2*l - 1), wp)*one_minus_x2**(0.5_wp*real(l, wp))
+ ! P_l^l via log-sum to avoid int64 overflow: (2l-1)!! = exp(sum_{k=1}^{l} log(2k-1))
+ block
+ integer :: k
+ real(wp) :: log_dfac
+ log_dfac = 0._wp
+ do k = 1, l
+ log_dfac = log_dfac + log(real(2*k - 1, wp))
+ end do
+ result_P = real((-1)**l, wp)*exp(log_dfac)*one_minus_x2**(0.5_wp*real(l, wp))
+ end blockAlternatively, accumulate P_l^l iteratively (starting from P_0^0 = 1 and applying the diagonal step l times), which is fully overflow-free and avoids the recursive call chain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/m_helper.fpp` at line 579, The expression computing result_P uses
double_factorial(2*l - 1) which returns int64 and overflows for l≥18; replace
the large-integer factorial path with an overflow-free computation: compute
P_l^l without calling double_factorial by either (a) using the same real-type
accumulation approach used in real_ylm (convert multipliers to real(wp) and
multiply progressively to form the diagonal term) or (b) iteratively accumulate
the diagonal recurrence starting from P_0^0 = 1 and applying the diagonal step l
times to produce result_P; update the code that sets result_P to use one of
these overflow-safe strategies instead of real(double_factorial(...), wp).
| impure subroutine s_check_2d_modal_patch_geometry(patch_id) | ||
| integer, intent(in) :: patch_id | ||
|
|
||
| call s_int_to_str(patch_id, iStr) | ||
|
|
||
| @:PROHIBIT(n == 0, "2D modal patch "//trim(iStr)//": n must be greater than zero") | ||
| @:PROHIBIT(p > 0, "2D modal patch "//trim(iStr)//": p must be zero") | ||
| @:PROHIBIT(patch_icpp(patch_id)%radius <= 0._wp, "2D modal patch "//trim(iStr)//": radius must be greater than zero") | ||
| @:PROHIBIT(f_is_default(patch_icpp(patch_id)%x_centroid), "2D modal patch "//trim(iStr)//": x_centroid must be set") | ||
| @:PROHIBIT(f_is_default(patch_icpp(patch_id)%y_centroid), "2D modal patch "//trim(iStr)//": y_centroid must be set") | ||
|
|
||
| end subroutine s_check_2d_modal_patch_geometry |
There was a problem hiding this comment.
Validate modal_r_min when clipping is enabled.
If modal_clip_r_to_min is true but modal_r_min is unset or negative, the clip is ineffective or ambiguous. Consider enforcing a valid minimum.
🛠️ Suggested validation
impure subroutine s_check_2d_modal_patch_geometry(patch_id)
@@
@:PROHIBIT(f_is_default(patch_icpp(patch_id)%y_centroid), "2D modal patch "//trim(iStr)//": y_centroid must be set")
+ if (patch_icpp(patch_id)%modal_clip_r_to_min) then
+ @:PROHIBIT(f_is_default(patch_icpp(patch_id)%modal_r_min), &
+ "2D modal patch "//trim(iStr)//": modal_r_min must be set when modal_clip_r_to_min is true")
+ @:PROHIBIT(patch_icpp(patch_id)%modal_r_min < 0._wp, &
+ "2D modal patch "//trim(iStr)//": modal_r_min must be non-negative")
+ end if🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pre_process/m_check_patches.fpp` around lines 253 - 264, In
s_check_2d_modal_patch_geometry add validation for
patch_icpp(patch_id)%modal_r_min when patch_icpp(patch_id)%modal_clip_r_to_min
is true: check that modal_r_min is set (not f_is_default) and greater than zero
(and optionally less than or equal to patch_icpp(patch_id)%radius) by adding
appropriate @:PROHIBIT checks that reference
patch_icpp(patch_id)%modal_clip_r_to_min and patch_icpp(patch_id)%modal_r_min to
fail fast when clipping is enabled but the minimum radius is unset or
non-positive.
| if ((r <= R_boundary .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, 0))) & | ||
| .or. patch_id_fp(i, j, 0) == smooth_patch_id) then | ||
| call s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf, patch_id_fp) | ||
| end if |
There was a problem hiding this comment.
Update patch_id_fp after assignments in 2D modal patches.
Without updating the patch ID bookkeeping, later alter_patch and smoothing logic can behave incorrectly.
🛠️ Suggested fix
if ((r <= R_boundary .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, 0))) &
.or. patch_id_fp(i, j, 0) == smooth_patch_id) then
call s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf, patch_id_fp)
+ if (1._wp - eta < sgm_eps) patch_id_fp(i, j, 0) = patch_id
end if🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pre_process/m_icpp_patches.fpp` around lines 1093 - 1096, After calling
s_assign_patch_primitive_variables for 2D modal patches you must update the
patch bookkeeping so subsequent checks (e.g. patch_icpp(...)%alter_patch(...)
and smoothing using smooth_patch_id) see the new owner; modify the branch that
currently calls s_assign_patch_primitive_variables(patch_id, i, j, 0, ...) to
also set patch_id_fp(i, j, 0) = patch_id immediately after the call so
patch_id_fp reflects the assignment.
| if ((r <= R_surface .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, k))) & | ||
| .or. patch_id_fp(i, j, k) == smooth_patch_id) then | ||
| call s_assign_patch_primitive_variables(patch_id, i, j, k, eta_local, q_prim_vf, patch_id_fp) | ||
| end if |
There was a problem hiding this comment.
Update patch_id_fp after assignments in 3D spherical-harmonic patches.
This bookkeeping is required for correct alter/smoothing behavior in subsequent patches.
🛠️ Suggested fix
if ((r <= R_surface .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, k))) &
.or. patch_id_fp(i, j, k) == smooth_patch_id) then
call s_assign_patch_primitive_variables(patch_id, i, j, k, eta_local, q_prim_vf, patch_id_fp)
+ if (1._wp - eta_local < sgm_eps) patch_id_fp(i, j, k) = patch_id
end if🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pre_process/m_icpp_patches.fpp` around lines 1153 - 1156, The code
currently calls s_assign_patch_primitive_variables for 3D spherical-harmonic
patches but does not update the bookkeeping array patch_id_fp, which breaks
later alter/smoothing logic; after the call to
s_assign_patch_primitive_variables (inside the same conditional that checks
patch_icpp(patch_id)%alter_patch and smooth_patch_id) set patch_id_fp(i, j, k)
to patch_id so the footprint reflects the new assignment, ensuring subsequent
tests against patch_id_fp, smooth_patch_id, and alter_patch behave correctly.
Claude Code ReviewHead SHA: bf1ed75 Files Changed: 22Changed file paths (top 15)
Summary of Changes
FindingsNo bugs or CLAUDE.md violations found. The mathematical implementation, array bounds, MPI broadcasts, and input validation are all correct. Improvement opportunities:
Review performed by Claude Code |
codeant-ai
bot
added
size:XXL
Claude Code ReviewPR #1163 — Fix/Add Patches 13 (2D modal) and 14 (3D spherical harmonics) Files Changed: 22Key paths:
Summary
FindingsCRITICAL — Dimension enforcement regression in
|
Review Summary by QodoAdd 2D modal Fourier patch (geometry 13) and fix 3D spherical harmonic patch (geometry 14)
WalkthroughsDescription• Add geometry 13: 2D modal (Fourier) patch with cos/sin modes (1–10) - Supports additive and exponential boundary forms - Optional radius clipping and smoothing • Fix geometry 14: Replace buggy spherical harmonic with real Y_lm coefficients - Explicit per-(l,m) coefficients (l=0..5, m=-l..l) - Safe theta/phi computation near centroid with small_radius guard • Refactor spherical harmonic math helpers - Replace unassociated_legendre and spherical_harmonic_func with real_ylm - Improve associated_legendre with domain guards and numerical stability • Add three example cases and update parameter registry and documentation Diagramflowchart LR
A["Geometry 13<br/>2D Modal Fourier"] -->|"Fourier series<br/>theta-dependent"| B["Boundary radius<br/>R(theta)"]
C["Geometry 14<br/>3D Spherical Harmonic"] -->|"Real Y_lm<br/>theta,phi-dependent"| D["Surface radius<br/>R(theta,phi)"]
E["Math Helpers"] -->|"real_ylm<br/>associated_legendre"| F["Robust<br/>Computation"]
B --> G["Patch Initialization"]
D --> G
F --> G
File Changes1. src/pre_process/m_icpp_patches.fpp
|
Code Review by Qodo
1. Sph-harm patch skips analytical IC
|
Claude Code ReviewHead SHA: e85160c Changed files
Summary
Findings🔴 CRITICAL — Missing namelist binding in
|
| Location | Status |
|---|---|
toolchain/mfc/params/definitions.py |
✅ Added |
src/pre_process/m_global_parameters.fpp |
✅ Initialized |
src/pre_process/m_start_up.fpp (namelist) |
❌ Missing |
toolchain/mfc/case_validator.py (constraints) |
Claude Code ReviewHead SHA: e85160c Changed files
Summary
Findings[CRITICAL]
|
📝 WalkthroughWalkthroughThis pull request extends patch geometry support from indices 1–13 to 1–21. New geometries are introduced: geometry 13 for 2D modal patches using Fourier series representations, and geometry 14 for 3D spherical harmonic patches. Supporting changes include new module constants ( 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
docs/documentation/case.md (1)
1105-1117:⚠️ Potential issue | 🟡 MinorAdd matching Geometry 14 narrative block (still missing).
Geometry 13 now has detailed semantics, but Geometry 14 still lacks a comparable explanation (coordinate convention, real-(Y_l^m) basis, and
sph_har_coeff(l,m)meaning), which makes configuration error-prone.📘 Suggested doc addition
**Geometry 13: 2D modal (Fourier):** ... - **Exponential form** (`modal_use_exp_form` true): \f$R_{\mathrm{boundary}} = \mathrm{radius} \times \exp\bigl( \sum_n [\ldots] \bigr)\f$. Coefficients are relative (dimensionless); the sum scales the radius. + +**Geometry 14: 3D spherical harmonic:** +Boundary is defined relative to centroid using \f$\theta=\arccos(z/r)\f$ and \f$\phi=\mathrm{atan2}(y,x)\f$, with +\f$R_{\mathrm{boundary}}=\mathrm{radius}+\sum_{l=0}^{5}\sum_{m=-l}^{l}\mathtt{sph\_har\_coeff}(l,m)\,Y_l^m(\theta,\phi)\f$. +Uses the real-\f$Y_l^m\f$ basis; coefficients are absolute (length units).src/pre_process/m_check_patches.fpp (1)
253-264:⚠️ Potential issue | 🟡 MinorValidate
modal_r_minwhen clipping is enabled.When
modal_clip_r_to_minis true,modal_r_minshould be explicitly validated (set and non-negative) to prevent ambiguous/invalid clipping behavior.🛠️ Suggested validation patch
impure subroutine s_check_2d_modal_patch_geometry(patch_id) @@ @:PROHIBIT(f_is_default(patch_icpp(patch_id)%x_centroid), "2D modal patch "//trim(iStr)//": x_centroid must be set") @:PROHIBIT(f_is_default(patch_icpp(patch_id)%y_centroid), "2D modal patch "//trim(iStr)//": y_centroid must be set") + if (patch_icpp(patch_id)%modal_clip_r_to_min) then + @:PROHIBIT(f_is_default(patch_icpp(patch_id)%modal_r_min), & + "2D modal patch "//trim(iStr)//": modal_r_min must be set when modal_clip_r_to_min is true") + @:PROHIBIT(patch_icpp(patch_id)%modal_r_min < 0._wp, & + "2D modal patch "//trim(iStr)//": modal_r_min must be non-negative") + end ifsrc/pre_process/m_icpp_patches.fpp (1)
1093-1096:⚠️ Potential issue | 🟠 MajorRestore standard post-assignment hooks in the new modal/spherical-harmonic branches.
Line 1095 and Line 1155 assign primitives but skip the usual
@:analytical()/@:Hardcoded*()override path used by other geometries, and skip the local patch ownership update pattern after full assignment. This makes geometries 13/14 behavior inconsistent with existing patch routines.🛠️ Proposed fix
subroutine s_icpp_2d_modal(patch_id, patch_id_fp, q_prim_vf) + @:HardcodedDimensionsExtrusion() + @:Hardcoded2DVariables() @@ if ((r <= R_boundary .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, 0))) & .or. patch_id_fp(i, j, 0) == smooth_patch_id) then call s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf, patch_id_fp) + @:analytical() + if (patch_icpp(patch_id)%hcid /= dflt_int) then + @:Hardcoded2D() + end if + if (1._wp - eta < sgm_eps) patch_id_fp(i, j, 0) = patch_id end if end do end do + @:HardcodedDellacation() end subroutine s_icpp_2d_modal subroutine s_icpp_3d_spherical_harmonic(patch_id, patch_id_fp, q_prim_vf) + @:HardcodedDimensionsExtrusion() + @:Hardcoded3DVariables() @@ if ((r <= R_surface .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, k))) & .or. patch_id_fp(i, j, k) == smooth_patch_id) then call s_assign_patch_primitive_variables(patch_id, i, j, k, eta_local, q_prim_vf, patch_id_fp) + @:analytical() + if (patch_icpp(patch_id)%hcid /= dflt_int) then + @:Hardcoded3D() + end if + if (1._wp - eta_local < sgm_eps) patch_id_fp(i, j, k) = patch_id end if end do end do end do + @:HardcodedDellacation() end subroutine s_icpp_3d_spherical_harmonicAlso applies to: 1153-1156
🧹 Nitpick comments (2)
src/common/m_constants.fpp (1)
18-30: Please run full target tests for thissrc/commonchange.Since these constants are shared broadly, run
./mfc.sh test -j 8to coverpre_process,simulation, andpost_processbefore merge.Based on learnings: Code in
src/common/affects all three executables (pre_process, simulation, post_process) and must be tested thoroughly.src/common/m_derived_types.fpp (1)
229-237: Please run the full 3-target test suite for thissrc/commonschema change.This type expansion impacts shared code paths across executables; run
./mfc.sh test -j 8before merge.As per coding guidelines: "For changes to src/common/, test ALL three targets by running ./mfc.sh test -j 8 since src/common/ is used by all three executables".
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
docs/documentation/case.mdexamples/2D_patch_modal_shape/case.pyexamples/2D_patch_modal_shape_exp/case.pyexamples/3D_patch_spherical_harmonic/case.pysrc/common/m_constants.fppsrc/common/m_derived_types.fppsrc/common/m_helper.fppsrc/pre_process/m_check_patches.fppsrc/pre_process/m_global_parameters.fppsrc/pre_process/m_icpp_patches.fppsrc/pre_process/m_mpi_proxy.fpptests/1967AE06/golden-metadata.txttests/1967AE06/golden.txttests/3F864CFF/golden-metadata.txttests/3F864CFF/golden.txttests/87FBC893/golden-metadata.txttests/87FBC893/golden.txttoolchain/mfc/case.pytoolchain/mfc/params/definitions.pytoolchain/mfc/params/descriptions.pytoolchain/mfc/params_tests/test_registry.py
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated 2D modal (Fourier) patch (geometry 13) and replaces the previous 3D spherical-harmonic patch with an explicit real-spherical-harmonics implementation (geometry 14), wiring both through the parameter registry, MPI broadcast, defaults, examples, and documentation.
Changes:
- Add geometry 13 (2D modal Fourier) with per-mode
fourier_cos/sinand options for exponential form + radius clipping. - Replace geometry 14 implementation with explicit real spherical harmonics driven by
sph_har_coeff(l,m)coefficients. - Update toolchain parameter registry/descriptions, pre_process patch validation/initialization, docs, and add example/test fixtures.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/params_tests/test_registry.py | Bumps registry-size expectation to account for newly registered params. |
| toolchain/mfc/params/descriptions.py | Adds human-readable descriptions for the new geometry 13/14 inputs. |
| toolchain/mfc/params/definitions.py | Registers new patch_icpp parameters for Fourier modes and spherical-harmonic coefficients. |
| toolchain/mfc/case.py | Updates dimensional patch-type mapping so geometry 13 is 2D-only and 14 is 3D-only. |
| tests/87FBC893/golden-metadata.txt | Adds generated golden metadata fixture for a new/updated test case. |
| tests/3F864CFF/golden-metadata.txt | Adds generated golden metadata fixture for a new/updated test case. |
| tests/1967AE06/golden.txt | Adds golden output data for a new/updated regression test. |
| tests/1967AE06/golden-metadata.txt | Adds generated golden metadata fixture for the regression test. |
| src/pre_process/m_mpi_proxy.fpp | Broadcasts the new patch components (fourier_*, sph_har_coeff, modal flags) over MPI. |
| src/pre_process/m_icpp_patches.fpp | Adds geometry 13 initializer and replaces geometry 14 initializer routine. |
| src/pre_process/m_global_parameters.fpp | Initializes defaults for the new patch parameters. |
| src/pre_process/m_check_patches.fpp | Adds geometry 13 validation and updates geometry 14 validation entry point. |
| src/common/m_helper.fpp | Introduces real_ylm and associated Legendre logic; removes old spherical-harmonic helpers. |
| src/common/m_derived_types.fpp | Extends ic_patch_parameters with Fourier mode arrays and sph_har_coeff(l,m). |
| src/common/m_constants.fpp | Adds max_2d_fourier_modes, max_sph_harm_degree, and small_radius. |
| examples/3D_patch_spherical_harmonic/case.py | New example demonstrating geometry 14 with explicit (l,m) coefficients. |
| examples/2D_patch_modal_shape_exp/case.py | New example demonstrating geometry 13 in exponential form. |
| examples/2D_patch_modal_shape/case.py | New example demonstrating geometry 13 in additive + clipping form. |
| docs/documentation/case.md | Updates patch-geometry docs and adds explicit documentation for geometry 13/14 inputs. |
| @:PROHIBIT(p > 0, "2D modal patch "//trim(iStr)//": p must be zero") | ||
| @:PROHIBIT(patch_icpp(patch_id)%radius <= 0._wp, "2D modal patch "//trim(iStr)//": radius must be greater than zero") | ||
| @:PROHIBIT(f_is_default(patch_icpp(patch_id)%x_centroid), "2D modal patch "//trim(iStr)//": x_centroid must be set") | ||
| @:PROHIBIT(f_is_default(patch_icpp(patch_id)%y_centroid), "2D modal patch "//trim(iStr)//": y_centroid must be set") |
There was a problem hiding this comment.
s_check_2d_modal_patch_geometry validates radius/centroid and dimensions, but it doesn’t validate modal_r_min when modal_clip_r_to_min is enabled. Since s_icpp_2d_modal uses modal_r_min in a max() clamp, a non-positive value would make the option ineffective or confusing. Please add a PROHIBIT to require modal_r_min > 0 when modal_clip_r_to_min is true (and consider also rejecting modal_clip_r_to_min when modal_use_exp_form is true, since it has no effect in that mode).
| @:PROHIBIT(f_is_default(patch_icpp(patch_id)%y_centroid), "2D modal patch "//trim(iStr)//": y_centroid must be set") | |
| @:PROHIBIT(f_is_default(patch_icpp(patch_id)%y_centroid), "2D modal patch "//trim(iStr)//": y_centroid must be set") | |
| @:PROHIBIT(patch_icpp(patch_id)%modal_clip_r_to_min .and. & | |
| patch_icpp(patch_id)%modal_r_min <= 0._wp, & | |
| "2D modal patch "//trim(iStr)//": modal_r_min must be greater than zero when modal_clip_r_to_min is enabled") | |
| @:PROHIBIT(patch_icpp(patch_id)%modal_clip_r_to_min .and. & | |
| patch_icpp(patch_id)%modal_use_exp_form, & | |
| "2D modal patch "//trim(iStr)//": modal_clip_r_to_min is not allowed when modal_use_exp_form is enabled (it has no effect)") |
| end if | ||
| if ((r <= R_boundary .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, 0))) & | ||
| .or. patch_id_fp(i, j, 0) == smooth_patch_id) then | ||
| call s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf, patch_id_fp) |
There was a problem hiding this comment.
In s_icpp_2d_modal, after calling s_assign_patch_primitive_variables the routine does not run the usual @:analytical()/hardcoded IC hooks and does not update patch_id_fp when eta≈1. Other patch geometries do both (so later alter_patch/smoothing logic can work correctly). Please add the same post-assignment steps here (and base the patch_id_fp update on eta/sgm_eps as in other 2D patches).
| call s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf, patch_id_fp) | |
| call s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf, patch_id_fp) | |
| @:analytical() | |
| @:hardcodedIC_2D() | |
| if (eta > 1._wp - sgm_eps) then | |
| patch_id_fp(i, j, 0) = patch_id | |
| end if |
| eta_local = 0.5_wp + 0.5_wp*tanh(smooth_coeff/min(dx, dy, dz)*(R_surface - r)) | ||
| end if | ||
| if ((r <= R_surface .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, k))) & | ||
| .or. patch_id_fp(i, j, k) == smooth_patch_id) then | ||
| call s_assign_patch_primitive_variables(patch_id, i, j, k, eta_local, q_prim_vf, patch_id_fp) | ||
| end if |
There was a problem hiding this comment.
In s_icpp_3d_spherical_harmonic, after s_assign_patch_primitive_variables the routine currently skips the standard @:analytical()/hardcoded IC handling and never updates patch_id_fp when eta_local≈1. This differs from other 3D patch initializers and can break later patch alternation/smoothing behavior. Please add the same post-assignment hooks and patch_id_fp bookkeeping update (using eta_local and sgm_eps).
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if ((r <= R_surface .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, k))) & | ||
| .or. patch_id_fp(i, j, k) == smooth_patch_id) then | ||
| call s_assign_patch_primitive_variables(patch_id, i, j, k, eta_local, q_prim_vf, patch_id_fp) | ||
| end if |
There was a problem hiding this comment.
1. Sph-harm patch skips analytical ic 🐞 Bug ✓ Correctness
Geometry 14 (s_icpp_3d_spherical_harmonic) assigns patch primitive variables but never calls the @:analytical() / @:Hardcoded3D() hooks used by other 3D patches, so analytical expressions (and HCID overrides) generated by the toolchain won’t be applied for this geometry.
Agent Prompt
### Issue description
Geometry 14’s patch routine (`s_icpp_3d_spherical_harmonic`) assigns primitive variables but does not invoke the fypp hook `@:analytical()` (and does not provide the usual `hcid` hardcoded hook). This means toolchain-generated analytical IC expressions for geometry 14 are never applied.
### Issue Context
`toolchain/mfc/case.py` generates a fypp macro `#:def analytical()` that writes analytical expressions into `q_prim_vf` for a given `patch_id`. Existing 3D patch routines invoke this macro via `@:analytical()` immediately after `s_assign_patch_primitive_variables`.
### Fix Focus Areas
- src/pre_process/m_icpp_patches.fpp[1150-1156]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd 2D modal (Fourier) patch (geometry 13) and fix 3D spherical harmonic patch (geometry 14)
WalkthroughsDescription• Add geometry 13: 2D modal (Fourier) patch with per-mode amplitudes - Supports additive and exponential forms with optional radius clipping • Fix geometry 14: Replace buggy spherical harmonic with robust real Y_lm implementation - Uses explicit sph_har_coeff(l,m) coefficients (l=0..5, m=-l..l) - Adds safe theta/phi computation near patch centroid • Refactor spherical harmonic math helpers with domain guards - Replace unassociated_legendre and spherical_harmonic_func with real_ylm - Improve associated_legendre with input validation and numerical safety • Add constants, parameter registry, validation, and three example cases Diagramflowchart LR
A["Geometry 13<br/>2D Modal Fourier"] -->|"Fourier series<br/>per mode"| B["Boundary radius<br/>R(theta)"]
C["Geometry 14<br/>3D Spherical Harmonic"] -->|"Real Y_lm<br/>coefficients"| D["Surface radius<br/>R(theta,phi)"]
B -->|"Additive or<br/>exponential form"| E["Patch shape"]
D -->|"Safe theta/phi<br/>computation"| E
F["Math helpers<br/>real_ylm, associated_legendre"] -->|"Domain guards<br/>numerical safety"| D
File Changes1. src/pre_process/m_icpp_patches.fpp
|
Code Review by Qodo
1. real_ylm missing f_ prefix
|
Claude Code ReviewHead SHA: a53c1a8 Files
Summary
Findings1. Missing lower-bound guard on
|
Claude Code ReviewHead SHA: a53c1a8 Summary
Findings🔴 Critical —
|
| unassociated_legendre, & | ||
| associated_legendre, & | ||
| spherical_harmonic_func, & | ||
| real_ylm, & |
There was a problem hiding this comment.
1. real_ylm missing f_ prefix 📘 Rule violation ✓ Correctness
A new public function real_ylm is introduced without the required f_ naming prefix for public functions. This breaks the project’s Fortran public API naming convention and can cause inconsistent interfaces.
Agent Prompt
## Issue description
A new public function `real_ylm` was added but does not follow the required `f_` prefix convention for public functions.
## Issue Context
`real_ylm` is exported from `m_helper` and is called from `m_icpp_patches`, so any rename must update both the public export list and all call sites.
## Fix Focus Areas
- src/common/m_helper.fpp[20-44]
- src/common/m_helper.fpp[526-548]
- src/pre_process/m_icpp_patches.fpp[1143-1148]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd 2D modal Fourier patch (geometry 13) and fix 3D spherical harmonic patch (geometry 14)
WalkthroughsDescription• Add geometry 13: 2D modal (Fourier) patch with cos/sin modes (1–10) - Supports additive and exponential forms - Optional radius clipping and minimum radius enforcement • Replace geometry 14: Fix 3D spherical harmonic patch with real Y_lm coefficients - Uses explicit sph_har_coeff(l,m) for l=0..5, m=-l..l - Eliminates division-by-zero near centroid with small_radius guard • Refactor spherical harmonic math helpers - Replace unassociated_legendre and spherical_harmonic_func with real_ylm - Improve associated_legendre with domain guards and numerical stability • Add three example cases and update parameter registry and documentation Diagramflowchart LR
A["Geometry 13<br/>2D Modal Fourier"] -->|"Fourier series<br/>cos/sin modes"| B["Boundary radius<br/>R(theta)"]
B -->|"Additive or<br/>Exponential"| C["2D patch shape"]
D["Geometry 14<br/>3D Spherical Harmonic"] -->|"Real Y_lm<br/>coefficients"| E["Surface radius<br/>R(theta,phi)"]
E -->|"Sum l,m<br/>contributions"| F["3D patch shape"]
G["Math helpers"] -->|"real_ylm<br/>associated_legendre"| H["Robust<br/>computation"]
H -->|"small_radius<br/>guard"| I["Avoid<br/>division-by-zero"]
File Changes1. src/pre_process/m_icpp_patches.fpp
|
Code Review by Qodo
1. Modal patch skips IC hooks
|
Claude Code ReviewHead SHA: a53c1a8 Changed files
Summary
Findings1. Critical —
|
| if (patch_icpp(patch_id)%smoothen) then | ||
| eta = 0.5_wp + 0.5_wp*tanh(smooth_coeff/min(dx, dy)*(R_boundary - r)) | ||
| end if | ||
| if ((r <= R_boundary .and. patch_icpp(patch_id)%alter_patch(patch_id_fp(i, j, 0))) & | ||
| .or. patch_id_fp(i, j, 0) == smooth_patch_id) then | ||
| call s_assign_patch_primitive_variables(patch_id, i, j, 0, eta, q_prim_vf, patch_id_fp) | ||
| end if |
There was a problem hiding this comment.
1. Modal patch skips ic hooks 🐞 Bug ✓ Correctness
s_icpp_2d_modal never executes the @:analytical() / hcid hardcoded-IC hooks after cell assignment, so analytical patch expressions and hardcoded ICs are silently ignored for geometry 13.
Agent Prompt
### Issue description
`src/pre_process/m_icpp_patches.fpp::s_icpp_2d_modal` assigns patch primitive variables but never calls the standard `@:analytical()` and `hcid`-gated hardcoded IC hooks that other patch geometries call after assignment. As a result, toolchain-generated analytical IC expressions and `patch_icpp(patch_id)%hcid` hardcoded IC behavior will be ignored for geometry 13.
### Issue Context
Other 2D/3D patch routines call `@:analytical()` immediately after `s_assign_patch_primitive_variables` and conditionally call `@:Hardcoded2D()`/`@:Hardcoded3D()` when `hcid` is set. Geometry 13 should behave consistently.
### Fix Focus Areas
- src/pre_process/m_icpp_patches.fpp[1046-1099]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
User description
Description
This PR fixes the 3D spherical harmonics and adds a dedicated 2D modal patch type:
Geometry 13 is 2D modal (Fourier), with explicit
fourier_cos/fourier_sin, optional additive vs. exponential form, and optional radius clipping.Geometry 14 is 3D spherical harmonics and uses real spherical harmonics with an explicit sph_har_coeff(l,m) surface definition. It replaces the old spherical-harmonic subroutine, which had buggy and undocumented behavior. There are no backward compatibility issues, as the original Geometry 14 was not used in any examples or tests. This PR adds clear examples and documentation.
Type of change
Testing
Checklist
See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)CodeAnt-AI Description
Add 2D Fourier modal patch (geometry 13) and correct 3D spherical-harmonic patch (geometry 14)
What Changed
Impact
✅ New 2D modal boundary shapes available for 2D cases✅ Clearer and correct 3D spherical-harmonic surfaces from explicit coefficients✅ Fewer numerical errors near patch centroids (no division-by-zero)💡 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.