Choose ib_neighborhood_radius from the thinnest rank, not the widest - #1866
Open
sbryngelson wants to merge 2 commits into
Open
Choose ib_neighborhood_radius from the thinnest rank, not the widest#1866sbryngelson wants to merge 2 commits into
sbryngelson wants to merge 2 commits into
Conversation
ib_neighborhood_radius counts rank hops: a rank keeps an immersed-boundary patch only while the centroid lies inside its subdomain grown by that many hops, and drops it otherwise. The radius therefore has to span the body in every direction, and the hop that needs the most is the one crossing the thinnest rank. The automatic choice assembled its rank width the wrong way round -- each rank reported its widest extent and the minimum was taken across ranks. Where every rank is long in one direction and thin in another, the width reported is the long one and the radius comes out too small. examples/3D_ibm_neighborhood_radius is a thin plate in a long narrow channel, sized so the topology search settles on 16 x 2 x 2 at 64 ranks: x ranks 1.250 wide against y and z at 4.000 and 3.000. The plate's half-extent is 1.3010, so crossing it takes two hops of 1.250 and one of 4.000. MFC prints radius 1 before this change and 2 after; both runs complete. The two production grids that motivated it agree: a gust case at 16 x 2 x 4 moves 1 -> 2, and a flapping wing at 8 x 4 x 4 moves 1 -> 2. The new width is never larger than the old, so the radius never decreases -- the change can only make the neighbourhood more conservative. Cases that set the parameter explicitly are untouched, as is any near-cubic decomposition, where the widest and narrowest extents coincide.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes the automatic selection of ib_neighborhood_radius for anisotropic MPI decompositions by basing the hop distance on the thinnest rank extent, and adds a reproducer example documenting/validating the behavior.
Changes:
- Change rank-width reduction logic from “widest direction per-rank” to “narrowest direction per-rank” when computing the auto
ib_neighborhood_radius. - Add a new 3D example case that reproduces the issue and prints a quick SUMMARY of the expected arithmetic.
- Document the reasoning, before/after result, and scope in an accompanying README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/simulation/m_start_up.fpp | Fixes the rank-width computation used to auto-select ib_neighborhood_radius on anisotropic decompositions. |
| examples/3D_ibm_neighborhood_radius/case.py | Adds a minimal reproducer case (and optional SUMMARY output) to demonstrate expected radius selection. |
| examples/3D_ibm_neighborhood_radius/README.md | Documents the issue, reasoning, and validation results for the new reproducer case. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+30
to
+34
| | direction | ranks | extent per rank | | ||
| | --- | --- | --- | | ||
| | x | 16 | **1.250** | | ||
| | y | 2 | 4.000 | | ||
| | z | 2 | 3.000 | |
Comment on lines
+52
to
+55
| | | printed radius | | ||
| | --- | --- | | ||
| | before | **1** | | ||
| | after | **2** | |
Comment on lines
+64
to
+67
| | case | topology | old width | old radius | new width | new radius | | ||
| | --- | --- | --- | --- | --- | --- | | ||
| | gust encounter, 128 ranks | 16 x 2 x 4 | 2.051 | 1 | 1.052 | **2** | | ||
| | flapping wing, 128 ranks | 8 x 4 x 4 | 1.745 | 1 | 1.027 | **2** | |
Comment on lines
+129
to
+130
| print(f"rank extents at 64 ranks (16 x 2 x 2): x {(x1 - x0) / 16:.3f}, " f"y {(y1 - y0) / 2:.3f}, z {(z1 - z0) / 2:.3f}") | ||
| print(f"hops needed across the thinnest rank: ceil(1.1 * {bound:.4f} / {(x1 - x0) / 16:.3f}) = " f"{max(1, math.ceil(1.1 * bound / ((x1 - x0) / 16)))}") |
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
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.
Written with Claude Code. Addresses the automatic-choice half of #1851.
The bug
ib_neighborhood_radiuscounts rank hops. A rank keeps an immersed-boundary patch only while the patch centroid lies inside its own subdomain grown outward by that many hops (s_get_neighbor_bounds→f_neighborhood_ranks_own_location), ands_handoff_ib_ownershipdeletes it otherwise. So the radius must span the body in every direction, and the hop that needs the most is the one crossing the thinnest rank.The automatic choice assembled its rank width the wrong way round:
Each rank reports its widest extent; the minimum is then taken across ranks. Where every rank is long in one direction and thin in another, the width reported is the long one and the radius comes out too small — the reported "picks 1 and faults".
The fix
Reduce over directions with
minrather thanmax, so the result is the narrowest rank extent over every rank and every direction. Two lines.How the validation is obtained
examples/3D_ibm_neighborhood_radius/— a thin plate in a long narrow channel, 20 chords by 8 by 6 with 400×50×50 cells, sized so MFC's topology search settles on 16×2×2 at 64 ranks. That is an ordinary shape for a wake, a jet or a channel, and it makes the ranks strongly anisotropic:The plate is 1.0 × 2.4 × 0.1, so
s_get_ib_boundreturns 1.3010. Crossing that at 1.250 per hop needsceil(1.1 × 1.3010 / 1.250) = 2; the old width of 4.000 gives 1.Automatic choice of ib_neighborhood_radius selected:Both runs complete (1 M cells, a few minutes on two CPU nodes).
SUMMARY=1 python3 case.pyprints the half-extent, the rank extents and the arithmetic without running anything. The README carries the same table and the reasoning.Two real production grids agree, measured from their own
lustre_*_cb.dat:Scope
The new width is never larger than the old, so the chosen radius never decreases — the change can only make the neighbourhood more conservative, at the cost of more hops in the force reduction. Cases that set
ib_neighborhood_radiusexplicitly are untouched, and so is any near-cubic decomposition, where the widest and narrowest extents coincide. That is why a uniform grid with a balanced topology shows no difference, and why no golden file moves.This does not fix #1863 (a spurious transverse force on multi-rank IB runs); I checked, and raising the radius there changes nothing.