Skip to content

Unify geometry IDs for Immersed Boundaries and ICPP Fixes #1543#1547

Open
Riship749 wants to merge 5 commits into
MFlowCode:masterfrom
Riship749:master
Open

Unify geometry IDs for Immersed Boundaries and ICPP Fixes #1543#1547
Riship749 wants to merge 5 commits into
MFlowCode:masterfrom
Riship749:master

Conversation

@Riship749

Copy link
Copy Markdown

This PR unifies the redundant 2D and 3D geometry integers into single, dimension-agnostic IDs within the pre-processor for both patch_ib and patch_icpp.

Changes:

  • Merged Rectangles (3) and Cuboids (9) -> ID 3.
  • Merged 2D Airfoils (4) and 3D Airfoils (11) -> ID 4.
  • Merged Circles (2), Spheres (8), and Cylinders (10) -> ID 2.

The unified subroutines now dynamically check for 3D constraints (e.g., Z-centroids and Z-lengths) using the p > 0 condition. Legacy compatibility is maintained by routing the old 3D IDs directly to the newly unified subroutines. Obsolete 3D-specific subroutines have been removed to reduce code duplication.

Testing:
Compiled successfully locally and passed the full ./mfc.sh test regression suite.

Fixes #1543

@sbryngelson sbryngelson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice dedup direction — collapsing the near-identical 2D/3D check subroutines is worthwhile. But the unification is applied to the validation layer only, while patch generation (m_icpp_patches.fpp, IB equivalent) still dispatches on the original IDs (2/3/8/9/10/11). That mismatch turns several of the simplified checks into silent validation gaps:

  1. 3D IDs no longer rejected in lower-D runs — the original p == 0 prohibits on sphere/cuboid/cylinder/3D-airfoil are gone, so e.g. geometry = 8 in a 2D case passes validation but hits the 3D generator.
  2. ICPP circle dropped its n == 0 guard — a circle in a 1D run is now accepted (IB + rectangle checks still keep this guard, so it looks accidental).
  3. Cylinder length validation weakened — gating "exactly one length" behind "any length > 0" lets a no-length or negative-length cylinder through.

Details inline. The "compiles + ./mfc.sh test passes" result is expected — these are all gaps in rejecting bad input, which a suite of valid cases won't exercise. Worth adding (or confirming) a case that feeds a 3D ID into a 2D run.

Smaller nits (typos / missing space in messages) also inline.

Comment thread src/pre_process/m_check_patches.fpp
Comment thread src/pre_process/m_check_patches.fpp
Comment thread src/pre_process/m_check_patches.fpp Outdated
Comment thread src/pre_process/m_check_ib_patches.fpp
Comment thread src/pre_process/m_check_ib_patches.fpp Outdated
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 28.88889% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.87%. Comparing base (1f02a6f) to head (9313ce9).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
src/pre_process/m_check_ib_patches.fpp 13.33% 8 Missing and 5 partials ⚠️
src/common/m_helper_basic.fpp 28.57% 9 Missing and 1 partial ⚠️
src/pre_process/m_check_patches.fpp 10.00% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1547      +/-   ##
==========================================
+ Coverage   60.57%   60.87%   +0.30%     
==========================================
  Files          73       73              
  Lines       20264    20176      -88     
  Branches     2949     2946       -3     
==========================================
+ Hits        12275    12283       +8     
+ Misses       6000     5891     -109     
- Partials     1989     2002      +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Riship749

Copy link
Copy Markdown
Author

The generation dispatchers in m_icpp_patches and m_ib_patches are now fully unified and handle the legacy IDs natively based on the p > 0 and n > 0 dimensional blocks. I also added the count and n==0 validator checks you requested.
Codecov might still flag the new @:PROHIBIT lines since the standard test suite doesn't feed bad dimension/length inputs to trigger them.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR unifies 2D/3D patch geometry IDs for ICPP and immersed boundary (IB) patches in the pre-processor, routing legacy 3D IDs to the unified implementations and reducing duplicated geometry handling.

Changes:

  • Unified geometry-ID dispatch for sphere/cylinder/circle (2/8/10), cuboid/rectangle (3/9), and swept patches (4/11) in ICPP patch application.
  • Updated patch-geometry validation logic to accept unified/legacy IDs and apply 3D-only constraints when p > 0.
  • Removed now-redundant 3D-specific geometry-check subroutines for ICPP and IB.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/pre_process/m_icpp_patches.fpp Updates ICPP geometry dispatch to accept unified + legacy IDs and route to unified shape routines.
src/pre_process/m_check_patches.fpp Updates ICPP geometry validation dispatch/constraints for unified + legacy IDs, with 3D checks gated by p > 0.
src/pre_process/m_check_ib_patches.fpp Updates IB geometry validation dispatch/constraints for unified + legacy IDs, with 3D checks gated by p > 0.

Comment on lines +59 to 63
else if (patch_icpp(i)%geometry == 3 .or. patch_icpp(i)%geometry == 9) then
call s_check_rectangle_patch_geometry(i)
else if (patch_icpp(i)%geometry == 4) then
call s_check_line_sweep_patch_geometry(i)
else if (patch_icpp(i)%geometry == 5) then
Comment thread src/pre_process/m_check_patches.fpp Outdated
Comment thread src/pre_process/m_icpp_patches.fpp Outdated
Comment thread src/pre_process/m_check_ib_patches.fpp Outdated
@sbryngelson

Copy link
Copy Markdown
Member

Thanks for taking this on, @Riship749 — the direction here was right, and I've pushed a follow-up commit (e91aa2b) onto your branch that builds on it. A summary of what changed and why:

The main issue was that the unification only existed in the pre-processor's checkers. The simulation side (m_ib_patches.fpp, m_compute_levelset.fpp, and the moment-of-inertia code in m_ibm.fpp) still dispatches on the old strict 2D/3D IDs, so e.g. a 3D case with patch_ib(1)%geometry = 2 passed all checks but then silently produced no immersed boundary at all — the worst kind of failure for a CFD code, since the run completes and looks plausible.

Rather than teaching every downstream dispatch site about the new IDs, the follow-up collapses unified/legacy IDs to one canonical ID per dimensionality at a single point — f_canonical_geometry, called right after the case file is read in both pre_process and simulation. Everything downstream then works unchanged, which also let me:

  • revert the m_icpp_patches.fpp dispatch edits (no longer needed), including the icpp 4/11 merge — those are line/plane sweeps in the patch_icpp namespace (not airfoils, that's patch_ib), they take mutually exclusive parameters, and the checkers made those branches unreachable anyway;
  • restore the cylinder strictness that got lost: geometry 10 with no lengths (or a negative one) is a hard error again instead of silently becoming a sphere, keyed on the canonical ID;
  • extend the unification to the airfoil (4/11) and STL model (5/12) IB pairs, completing the list in Unify 2D and 3D Patch Geometries #1543;
  • add two golden tests declaring a 3D IB sphere and cylinder via geometry 2 — the unified-sphere golden is bit-identical to the legacy geometry-8 case, which is a nice end-to-end proof the canonicalization works.

Net effect vs master is −41 source LOC with the docs tables updated to match. Full test suite passes locally apart from a few chemistry cases that failed for environmental reasons unrelated to this PR; CI will confirm.

…der checks

The unified/legacy geometry IDs are now collapsed to one canonical ID per
dimensionality (f_canonical_geometry) right after the case file is read, in
both pre_process and simulation. This makes the unification hold across the
whole pipeline (checks, ICPP application, IB markers, levelsets, moments of
inertia) instead of only in the pre-process checkers, and extends it to the
airfoil (4/11) and STL model (5/12) IB pairs from MFlowCode#1543.

- Revert the m_icpp_patches dispatch changes: with canonical IDs the original
  dispatch is already correct, and the icpp 4/11 (line/plane sweep) merge was
  unreachable since the checkers still enforce their dimensionality.
- Restore the cylinder strictness lost in the previous commit: geometry 10
  requires exactly one positive length again (a cylinder with no or negative
  lengths is a hard error, not a silent sphere), keyed on the canonical ID.
- Extend the toolchain bounding-box z-check to unified ID 3 and update the
  patch-type tables in the docs.
- Add two golden tests: a 3D IB sphere and cylinder declared via unified
  geometry 2 (the sphere golden is bit-identical to the legacy geometry-8
  sphere case).
@sbryngelson

Copy link
Copy Markdown
Member

@Riship749 heads up — your force-push earlier today removed the follow-up commit I had pushed onto this branch (see the comment above). I've re-applied it on top of your amended commit (now 68891386). Please pull before pushing again (git pull --rebase) so we don't lose it a second time.

On your amendments: the tightened two-step cylinder length check is covered by the re-applied commit (equivalent pair of checks, keyed on the canonical ID so that a cylinder with no lengths is also an error). I deliberately did not keep the line-sweep checker relaxation (allowing p > 0 for geometry 4): the icpp 4/11 pair are line/plane sweeps with mutually exclusive parameters (normal(3) forbidden vs required), so they can't be unified the way the circle/sphere/cylinder family can — the follow-up commit reverts that part and keeps icpp 4 and 11 as separate shapes. The airfoil 4/11 unification from #1543 lives in the patch_ib namespace and is handled by the canonicalization.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.

Comment on lines +1153 to +1154
| 2 | Circle | 2 & 3 | Y | Requires `[x,y]_centroid` and `radius`. In 3D, equivalent to 10 (cylinder) if a `length_[x,y,z]` is set and to 8 (sphere) otherwise. |
| 3 | Rectangle | 2 & 3 | N | Coordinate-aligned. Requires `[x,y]_centroid` and `length_[x,y]`. In 3D, equivalent to 9 (cuboid). |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Unify 2D and 3D Patch Geometries

3 participants