Fix/Add Patches 13 (2D modal) and 14 (3D spherical harmonics)#1163
Open
ChrisZYJ wants to merge 8 commits intoMFlowCode:masterfrom
Open
Fix/Add Patches 13 (2D modal) and 14 (3D spherical harmonics)#1163ChrisZYJ wants to merge 8 commits intoMFlowCode:masterfrom
ChrisZYJ wants to merge 8 commits intoMFlowCode:masterfrom
Conversation
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughAdds 2D modal (Fourier) and 3D spherical-harmonic patch geometries across docs, examples, Fortran core (types, constants, helpers, init/validation, MPI), toolchain param definitions, and new golden test data; includes example case generators that emit JSON configurations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Case as Example case file
participant Parser as Parameter parser
participant Validator as m_check_patches
participant MPI as m_mpi_proxy
participant Init as m_icpp_patches
participant Math as m_helper
participant Grid as Mesh initializer
Case->>Parser: Load `patch_icpp(i)` (geometry, coeffs, modal flags)
Parser->>Validator: Validate geometry and parameter ranges
Validator-->>Parser: Validation result
Parser->>MPI: Broadcast patch params (fourier_*, sph_har_coeff, modal_*)
MPI->>Init: Deliver parameters to all ranks
Init->>Math: Evaluate Fourier terms or Y_lm(θ,φ,l,m)
Math-->>Init: Return basis values
Init->>Grid: Compute R_boundary/surface and populate patch state
Grid->>Grid: Finalize initialization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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}
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.
Contributor
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.05% 44.43% +0.37%
==========================================
Files 70 70
Lines 20496 20502 +6
Branches 1989 1995 +6
==========================================
+ Hits 9030 9110 +80
+ Misses 10328 10244 -84
- Partials 1138 1148 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@sbryngelson I think this PR is ready. Could you please take a look? Thank you. |
Member
|
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) |
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
Contributor
|
CodeAnt AI Incremental review completed. |
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
Contributor
|
CodeAnt AI Incremental review completed. |
Contributor
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)) |
Contributor
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)) |
Contributor
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).
Comment on lines
+253
to
+264
| 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 |
Contributor
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.
Comment on lines
+1093
to
+1096
| 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 |
Contributor
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.
Comment on lines
+1153
to
1156
| 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 |
Contributor
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
✅ Can create 2D modal (Fourier) boundaries✅ Clearer, standard 3D spherical-harmonic surfaces✅ Fewer center-singularity errors for spherical patches💡 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.
Summary by CodeRabbit
New Features
Documentation
Tests