One boundary back-end, on the corner graph - #32
Open
hdrake wants to merge 3 commits into
Open
Conversation
NOT GREEN: 1 of 56 real-data tests fails. See the end. `sectionate` now resolves the corner topology of every grid, not just multi-tile ones, so the two back-ends collapse into one. `_single_tile_boundaries_from_mask` is gone, and with it `lockey` -- a rounded position on the unit sphere, used as a hash bucket, which meant this package and `sectionate` had to agree on a coincidence tolerance and did not (a 6.4 mm bin against a 1 mm metric). It also merged corners a grid distinguishes but places at one point, which is what a tripolar cap's singular meridian does to a whole column of them. The corner frame is now `o = 1` on every grid rather than depending on the staggering, since that is what the corner topology is indexed on. `grid_section` returning the same five values on every grid removes `_normalize_grid_section` and the `len(ij) > 2` guard: a single-tile grid is one face, so `f_c` is zeros rather than absent. The fold fixture is rebuilt. It used to place its seam row at `s(i) = min(i, -i)` in *both* longitude and latitude so that only genuine mirror pairs coincided -- which folds the coordinates back on themselves and leaves 3 of its 24 cells inside out. `sectionate` now says so rather than signing their velocity faces backwards. None of that contrivance is needed any more: identity comes from the declared fold, so the fixture is a pinched cylinder whose ellipses genuinely nest, and the seam's mirror pairs coincide because the geometry means it. `curve="latitude circle"` on an axis-aligned box now raises, because a meridional segment follows no parallel. That is the migration `sectionate` intended when it added `"latitude and great circle"`; the test uses it, and pins the raise. 48 passed, 10 skipped by default; 55 passed, 2 skipped with real data. **The known failure.** `test_ecco_hard_topology_regions_close_exactly[south_cap]` traces a boundary through a corner that LLC90 stores on no face. Declaring the southern boundary fold upstream recovers the 65E half -- those corners are stored on tile 0 -- and takes the un-stored count from 181 to 78, matching what the old coordinate matching produced. The 115W half is different: its corners fall only on tiles 9 and 12's high-X rows, which a 'left' staggering drops, so they are stored nowhere and no declaration can conjure them. The old implementation left them un-stored too, so how its tracer avoided routing through one is the thing to find out next. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`sectionate` now resolves the corner topology of every grid, not just multi-tile ones, so the two back-ends collapse into one. `_single_tile_boundaries_from_mask` goes, and with it `lockey` -- a rounded position on the unit sphere used as a hash bucket, which meant this package and `sectionate` had to agree on a coincidence tolerance and did not (a 6.4 mm bin against a 1 mm metric). It also merged corners a grid distinguishes but places at one point, which is what a tripolar cap's singular meridian does to a whole column of them. Nothing was lost with it. Comparing the old coordinate-merged graph against the new declared one on ECCO LLC90: same 105302 nodes, same 78 corners stored on no face, **zero** partition mismatches in either direction, zero nodes with a different native index -- and 235 nodes that *gain* edges. The corner frame is now the 'outer' lattice on every grid, which is what the corner topology is indexed on. It used to depend on the staggering, and that was a bug rather than a detail: on a 'right'-staggered single-tile grid a region wrapping the periodic seam traced as **two** loops with a spurious seam face and a budget that did not close. It is one loop, eight faces, and closes. Nothing covered that path -- every single-tile fixture here is 'outer' -- so there is now a 'right' fixture, and both its tests fail against the old frame. `grid_section` returning the same five values on every grid removes `_normalize_grid_section` and the `f_c is None` guards. `ij=` correspondingly takes `(i_c, j_c, f_c)`; it used to be documented as a 2-tuple and raise `IndexError` on one, and now says what it wants. The fold fixture is rebuilt. It placed its seam row at `s(i) = min(i, -i)` in *both* longitude and latitude so that only genuine mirror pairs coincided, which folds the coordinates back on themselves and leaves 3 of its 24 cells inside out; `sectionate` now refuses that rather than signing their velocity faces backwards. None of the contrivance is needed any more -- identity comes from the declared fold -- so it is a pinched cylinder whose ellipses nest. It keeps the same three mirror pairs and gains the five periodic-wrap pairs the old one was missing. `curve="latitude circle"` on an axis-aligned box now raises, because a meridional segment follows no parallel. That is the migration `sectionate` intended when it added `"latitude and great circle"`; the test uses it and pins the raise. The ECCO loader declares LLC90's southern boundary fold, which `face_connections` cannot express -- the same declaration as upstream's, in this package's own copy of the loader. Without it the southern cap's boundary runs along an unmerged lip and cannot close. Known and now documented rather than silent: `_pad_center` reads the grid's own metadata, so it does not see declared identifications. Cells either side of such a seam look unconnected to `connected_components`, which would split a component spanning it. Budgets still close -- the tracer annihilates the doubly-traced edges within one trace -- so this is region *identity*, not conservation, and on LLC90 the cells concerned are Antarctic land. It becomes live for any declared seam that carries flow; the fix is to source adjacency from the corner topology, which has seen them. Also: `data/*.nc` is gitignored, after a previous commit here tracked seventeen absolute symlinks into another checkout; the sectionate floor names the release this needs rather than the superseded one; and CLAUDE.md's architecture section describes the code that exists. 48 passed, 10 skipped; 60 passed with `REGIONATE_REALDATA_TESTS=1`. All six notebooks re-executed: no figure and no numerical output changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up, and the one thing that could still fail opaquely. `GriddedRegion.__init__` now requires a face index, but the three `open_gr` load sites still filled `None` for a file written before `f_c` was persisted. Such a file describes a single-tile region, and a single-tile grid is one face -- so zeros is both the right value and the only one the constructor accepts. It loaded, then died on the next save with "different number of dimensions on data and dims: 0 vs 1", which is not a message anyone can act on. Three behaviour changes had no test and now do: that round trip, the `ij` 3-tuple requirement, and -- verified by reverting each fix -- both fail without it. Two documentation corrections worth more than their size. `_pad_center`'s docstring said it *"mirrors"* `CornerTopology`, which is backwards in exactly the way that matters: the topology sees declared identifications and `_pad_center` does not, and that divergence is the region-identity bug flagged last commit. It now says so where a reader will look, rather than only in CLAUDE.md. And this package has its own copy of the un-stored-corner message, which is the one `grid_boundaries_from_mask` users actually hit; like upstream's it now names the corner slot instead of printing `lon=nan`, which it did in essentially every firing. Also: `_is_fold_boundary` was imported under a name `xgcm.padding` does not export, so the fallback was always taken -- behaviourally identical, but the comment claimed otherwise; and `get_geo_corners` went unused when `lockey` did. 50 passed, 10 skipped; 62 passed with `REGIONATE_REALDATA_TESTS=1`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Targets
topology-overhaul(the branch behind #22), notmain.Downstream of hdrake/sectionate#15, which makes corner identity come from the grid's declared metadata rather than from coordinates, and gives every grid a corner topology instead of only multi-tile ones. Needs that branch installed; the
pyproject.tomlfloor names the release it will become.One back-end instead of two
_single_tile_boundaries_from_maskis gone, and with itlockey— around(xyz, 9)position on the unit sphere used as a hash bucket. Two problems with it. It meant this package andsectionatehad to agree on a coincidence tolerance and did not: a 6.4 mm quantisation bin here against a 1 mm metric there. And it merged corners a grid distinguishes but happens to place at one point, which is what a tripolar cap's singular meridian does to a whole column of them.Everything now stitches on the corner-node graph, which is uniform across a periodic wrap, a bipolar fold, rotated and reversed tile seams, and cube-vertex junctions alike.
Nothing was lost. Comparing the old coordinate-merged graph against the new declared one on ECCO LLC90: the same 105302 nodes, the same 78 corners stored on no face, zero partition mismatches in either direction, zero nodes with a different native index — and 235 nodes that gain edges.
A fix, not just a simplification
The corner frame
_trace_and_dropworks in used to depend on the staggering. It is now the'outer'lattice on every grid, which is what the corner topology is indexed on.That was a bug. On a
'right'-staggered single-tile grid, a region wrapping the periodic seam traced as two loops with a spurious seam face and a budget that did not close:Nothing covered that path — every single-tile fixture here is
'outer'-staggered — so there is a new'right'fixture, and both of its tests fail against the old frame.Consequences of the upstream API
grid_sectionreturning the same five values on every grid removes_normalize_grid_sectionand thef_c is Noneguards.f_cis present everywhere; a single-tile grid is one face, so it is zeros rather thanNone.ij=correspondingly takes(i_c, j_c, f_c)— it was documented as a 2-tuple and raisedIndexErroron one, and now says what it wants.curve="latitude circle"on an axis-aligned box now raises, because a meridional segment follows no parallel. That is the migrationsectionateintended when it added"latitude and great circle"; the test uses it, and pins the raise.Fixtures
fold_gridplaced its seam row ats(i) = min(i, −i)in both longitude and latitude so that only genuine mirror pairs coincided. That folds the coordinates back on themselves and leaves 3 of its 24 cells inside out —sectionatenow refuses that rather than signing their velocity faces backwards. None of the contrivance is needed any more, because identity comes from the declared fold, so it is a pinched cylinder whose ellipses genuinely nest. It keeps the same three mirror pairs and gains the five periodic-wrap pairs the old one was missing (it had them only at the first and last row, though a periodic grid's first and last corner columns are the same point at every row).The ECCO loader declares LLC90's southern boundary fold, which
face_connectionscannot express — its schema maps a whole tile edge to a whole tile edge, and that boundary folds onto itself across four of them. Without it the southern cap's boundary runs along an unmerged lip and cannot close.Known, documented, not fixed
_pad_centerreads the grid's own metadata, so it does not see identifications declared throughsectionate.topology.declare_identifications. Cells either side of such a seam look unconnected toconnected_components, which would split a component spanning it. Budgets still close — the tracer annihilates the doubly-traced edges within one trace — so this is region identity, not conservation, and on LLC90 the cells concerned are Antarctic land. It becomes live for any declared seam that carries flow. The fix is to source cell adjacency from the corner topology, which has seen them; that is a change worth making on its own.Verification
REGIONATE_REALDATA_TESTS=1.AI-assisted PR, disclosed per the project AI Usage Policy: commits carry
Co-Authored-By:trailers.