Fix show(field) + zipper BC validation for (distributed) tripolar grids#5489
Open
Fix show(field) + zipper BC validation for (distributed) tripolar grids#5489
Conversation
`show` calls `maximum` which creates a reduced Field with (Nothing, Nothing, Nothing) location. On pencil-decomposed (Rx > 1) tripolar grids, the Field constructor injected a DistributedZipper north BC for this reduced field, then `communication_buffers` called `has_fold_line(TY, Nothing)` which had no method. Two fixes, matching the existing pattern from distributed LatitudeLongitudeGrid: 1. `default_auxiliary_bc` returns `nothing` for Nothing y-location on tripolar grids 2. The MPI tripolar Field constructor respects `nothing` north BCs instead of overriding them with a zipper Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
|
Wait, can we partition tri-polar grids in x now? |
navidcy
added
user interface/experience 💻
distributed 🕸️
Collaborator
Author
Yes but only for pencil partition, so 2x2 is ok but 2x1 isn't. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5489 +/- ##
==========================================
+ Coverage 74.10% 74.17% +0.06%
==========================================
Files 403 403
Lines 23288 23285 -3
==========================================
+ Hits 17257 17271 +14
+ Misses 6031 6014 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
navidcy
reviewed
Apr 13, 2026
| if yrank == processor_size[2] - 1 && processor_size[1] == 1 | ||
| if isnothing(global_bcs.north) | ||
| # Reduced fields (Nothing location) have no north boundary | ||
| north_bc = local_bcs.north |
Member
There was a problem hiding this comment.
this is what happens with the else at the end though
Can we make it clearer?
Collaborator
Author
There was a problem hiding this comment.
Yes probably. I think this is the minimal way of fixing it. I guess something clearner with dispatch would be better. I'll think about it.
Collaborator
Author
There was a problem hiding this comment.
OK I went down a rabbit hole a bit (#5503), but I think the latest commits fix this and make this a bit cleaner.
Collaborator
Author
There was a problem hiding this comment.
PS: I updated the PR description to reflect that (see point 2. in the PR description)
…patch Replace if/else branching with dispatch on y-topology (`OneDFoldTopology` / `TwoDFoldTopology`) and incoming north BC. Extract `zipper_sign`, `with_north_bc`, and a tripolar-specific `inject_halo_communication_boundary_conditions`. Behavior is preserved across serial, slab, and pencil partitions (verified with MWE — see #5503). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validates that the north boundary of a (folded-y) tripolar grid is a Zipper / distributed communication BC / nothing, both at Field construction and at the top of tripolar regularize_field_boundary_conditions. Drops the now-dead location-based `sign(LX, LY)` fallback and simplifies `zipper_sign`. - Adds `FoldedTopology` union in Grids and the matching `validate_boundary_condition_topology` methods (mirroring the Periodic pattern), guarded on `side == :north`. - Accepts `DefaultBoundaryCondition` placeholders at the regularize-entry validate so the model construction path isn't rejected before placeholders resolve. - Passes `boundary_conditions = nothing` on the internal metric helper Fields inside the `TripolarGrid` constructor so their default BCs (NoFlux / Impenetrable on folded y) don't trip the new validator. - Documents the sign convention on `UPivot/FPivotZipperBoundaryCondition` and adds a `!!! info "North boundary condition"` admonition to `TripolarGrid`'s docstring. - Adds serial + distributed tests covering validation (TESTSET 4 in gadi_test_env, plus test/test_tripolar_grid.jl). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…h cleanup
Regularize no longer clobbers user-supplied north BCs. It only fills the
default Zipper when the north slot holds a `DefaultBoundaryCondition`
placeholder (the model-construction path). A user-supplied north BC flows
through untouched; invalid BCs are then caught downstream by Field's
`validate_boundary_condition_topology` (the Periodic-style check). This
removes the need to validate at regularize entry and to accept
`DefaultBoundaryCondition` in the FoldedTopology validator.
- Replace the regularize-entry validate call + unconditional override with
7-arg `regularize_boundary_condition` overloads that pass `sign` through
and dispatch on BC type × grid y-topology. `DefaultBC` + fold-north
yields the right Zipper/DistributedZipper; user BCs pass through.
- Refactor the fold-topology unions into a four-way split that matches
how each case is dispatched:
SerialFoldedTopology — serial tripolar
SlabFoldedTopology — distributed slab fold-north (Rx=1)
PencilFoldedTopology — distributed pencil fold-north (Rx>1)
DistributedFoldedTopology — Slab ∪ Pencil
FoldedTopology — Serial ∪ Distributed (used for north-BC validation)
All live in `Grids/grid_utils.jl` and are exported. The previously
duplicate `DistributedFoldTopology` in `field_boundary_conditions.jl`
is deleted in favor of the shared `DistributedFoldedTopology`.
- Add short dispatch aliases (`SerialFTG`, `SlabFTG`, `PencilFTG`,
`DistFTG`) to keep method signatures readable.
- Update `north_zipper_bc` and `distributed_zipper.jl` dispatch to use
the renamed unions (`SlabFoldedTopology`, `DistributedPencilFoldedTopology`
→ `PencilFoldedTopology`).
- Update the test so it only exercises Field's validation path (Alt B
design: regularize deliberately doesn't throw on bad input).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briochemc
changed the title
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.
Two fixes for distributed tripolar grids:
show(field)crashes on 2×2 pencil decompositions. This is becauseshowcalls reduced fields (to show the min/max, with locNothing) and hitshas_fold_line(::TY, ::Nothing)which does not exist. This PR adds the needed methods.Zipper north BC on tripolar grids are not enforced (Tripolar grids north BC validation #5503). A user-supplied non-zipper north BC was silently discarded (distributed) or silently used (serial, producing wrong halos). Now in this PR:
validate_boundary_conditionsrejects any non-zipper/DCBC/nothing on a folded-y direction (newFoldedTopologyunion, mirrors thePeriodicvalidation pattern).regularize_field_boundary_conditionsonly fills the default Zipper when the user omitted the north BC (DefaultBoundaryConditionplaceholder); user-supplied BCs flow through untouched.sign(LX, LY)fallback removed;zipper_signcollapsed to a 2-method dispatch.FoldedTopologysplit intoSerialFoldedTopology/SlabFoldedTopology/PencilFoldedTopology/DistributedFoldedTopologyfor clarity.Closes #5503
🤖 Generated with help from Claude Code