Review follow-up: wording, docstrings, ECCO record v3.0.0, generic topology check - #9
Conversation
…ogy check Documentation and comment fixes from review 4899521952, plus two small behaviour changes that came out of it. - examples/load_example_ECCO_grid.py: point at the current Zenodo release (v3.0.0, record 21479854). The module docstring now names only the version-independent concept DOI and defers to ZENODO_RECORD_ID for the exact version, instead of restating a version DOI in prose. v3.0.0 concatenates each collection into one file per collection, so the twelve monthly volume-flux entries collapse to a single 2010 file; its published MD5 is recorded and the geometry file's checksum is unchanged. - gridutils.py: drop the "clip-to-edge" jargon in favour of plain language about closed boundaries (both occurrences), mark where the single-tile branch of build_neighbor_maps begins, define "grid cut" in the _OuterTopology docstring, and reframe that class's LLC90-specific remarks as illustrations of general statements. Measured tolerances are kept, now explicitly labelled as LLC90 measurements. Comments only. - transports.py: explain in _in_velocity_range what the check is for -- it is how _uv_for_edge decides whether a seam edge's velocity is stored on the source face or the destination face -- and handle "U" explicitly, so an unexpected component raises instead of silently taking the U branch. - section.py: _check_supported_topology no longer describes the north fold in terms of an xgcm face_connections representation that was never released. It now checks generically that the multi-tile topology is one sectionate can trace: every neighbour face exists, every connection is mutual, and no face is glued to itself. Verified that a self-glued face is not caught anywhere downstream (it silently traces a section as if the seam were absent), and that unknown/one-sided connections previously failed with a KeyError/TypeError from inside xgcm's padding. - CLAUDE.md: apply the requested edits to the project overview and the development-setup section. Tests: pin the self-glued, unknown-face and one-sided-connection failures, check that supported topologies (rotated seam, LLC90's 13-tile connections) still pass, and cover _in_velocity_range including the new raise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answering the reviewer's two questions about `gridutils.py` turned up documentation that did not survive measurement. `_OuterTopology`'s corner/outer-lattice guard now says what it actually catches. It is unreachable on a well-formed grid -- corner dims are Nc+1 for 'outer' and Nc for 'left'/'right', so the test is an equality for 'outer' and 'right' and leaves one slot of slack for 'left'. It fires only when the declared corner position contradicts the corner arrays' length, or when centers and corners come from different grids; xgcm does not validate dim lengths, so such a grid builds without complaint. Its message was also wrong for the commonest trigger -- a symmetric array declared 'right' is not "larger than the outer lattice", it just cannot sit at the +1 offset 'right' implies -- so it now prints both shapes and names the likely cause. `padded_transports` claimed that no halo pad can supply a value for edges stored on no face. Measured against the pinned xgcm that is not what happens: on ECCO LLC90 it and a dict-form vector pad agree bit for bit, both summing cell convergence to exactly 0.0 globally, because the slots it zeroes are exactly the ones the pad fills with a `fill_value` that is already 0 there. The docstring now claims only what holds: independence from the axis `fill_value` (on the cubed-sphere fixture, `fill_value=nan`, a vector pad leaves 23 of 128 cells' convergence NaN where a wall's transport is 0 by definition), independence from halo correctness across rotated seams, and that it takes plain arrays. CLAUDE.md's matching claim that the dict form of `xgcm.pad` "was always exact" is softened for the same reason: xgcm#712 was a general `_pad_face_connections` bug that could corrupt any face-connection pad, dict vector form included, while #749 fixed only the bare-DataArray path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An audit of this branch found several statements that are not true of the code, plus a topology check that rejects a legal grid. Fix all of them. `_check_supported_topology` claimed that face existence and connection mutuality are things "neither of which xgcm enforces when the grid is built". xgcm enforces both, at `Grid.__init__`: naming a face the grid does not have raises KeyError, and a one-sided connection raises TypeError, each while it builds its face-connection table. Only the self-gluing rejection is load bearing -- xgcm accepts a face glued to itself and nothing downstream notices, so a section is traced as though the seam were absent. The docstring now says that, and labels the other two branches as the defence in depth they are. The two tests that fired them by corrupting `grid._face_connections` after construction are replaced by one that asserts xgcm rejects those grids at construction, which is where the real enforcement lives. The same function built its set of known faces as `range(n_faces)`, so a grid labelled `face = [1, 2]` -- legal, and accepted by xgcm -- was rejected with "only has 2 faces" even though face 2 exists. Read the labels instead, and report them in the message. Covered by a new test. `padded_transports` claimed to be independent of "the halo being right across a rotated or reversed seam". It is not: `_OuterTopology` builds its whole node graph by padding tracer-cell ids with the grid's own `face_connections` via `xgcm.padding.pad`, and `padded_transports` reads that graph. What it really buys over a *vector* pad is that it needs no vector rotation, takes plain arrays, and is independent of the axis `fill_value`. The list also promised three things and gave four. Separately, a zeroed cut lip was justified as "a wall carries no transport" -- true for a wall, but for a cut lip or an un-stored cap vertex the edge exists and zero is the conservative choice, not a truth. Say so. The corner-array guard rendered as "Corner dims (9, 9) do not fit the outer lattice (9, 9)" for the very case it was written for -- two identical shapes and "do not fit" -- and leaked "outer lattice", a term that appears in no user-facing doc and is defined only inside the private `_OuterTopology`. Rewritten in terms of the corner and centre array shapes and the offset the declared staggering implies. Its twelve-line comment above a two-line `if` is trimmed. `_in_velocity_range` said a seam edge "is stored on only ONE of the two faces it touches"; `_uv_for_edge`'s third branch exists precisely for edges stored on neither. Corrected to "at most one", with that case named. The LLC90 generalisation begun on this branch stopped at the class body: the `gridutils.py` module header and the CLAUDE.md bullet still stated polar-cut specifics as general truth. Generalised, but not over-generalised -- a corner loses a cell at a cut that reaches a pole, not at "a grid cut or a pole", since a bipolar fold alone loses none. Per project policy, no source file references a merged PR or a closed issue outside a TODO; the four such references introduced here are replaced by descriptions of the behaviour or bug. (`test_section_fold.py` is left alone; its references are handled elsewhere.) Nits: define "fingerprint" and "seam twin" at first use, "bit-reversed" -> "index-reversed", trim the ten-line grid-cut definition, and fix the leftovers in `examples/load_example_ECCO_grid.py` from the twelve-files-to-one change -- including `load_ECCO_MOC_grid` claiming its transports are "time-averaged over the twelve months of 2010" when `utr`/`vtr` keep a length-12 `time` dimension. Full suite: 81 passed, 0 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Independent review findings, and what changedAn independent reviewer audited this PR for comment correctness and jargon. Its blocking 1. The Only the self-gluing rejection is load-bearing: xgcm accepts a face glued to itself and nothing 2. A legal grid was rejected. 3. Also corrected there: "three things" was followed by four, and zeroing an un-stored grid-cut lip 4. The corner-guard error contradicted itself. For the case it was written for — a symmetric 5. Smaller ones: 6. No links to merged PRs or closed issues anywhere in source, per project rule — only TODOs Full suite: 81 passed, 0 skips. The Written by Claude Code. |
`_check_supported_topology` re-checked two things `xgcm.Grid.__init__` already enforces: that every face named as a neighbour exists (an unknown one raises KeyError) and that every connection is mutual (a one-sided one raises TypeError). Both abort while xgcm builds its face-connection table, so no grid reaching this function could ever fail them. Keeping them as a backstop only guards against a `_face_connections` dict corrupted after construction, which is the caller's problem, not something to carry code for. Remove both branches, along with the `faces` label set that existed only to serve the existence check and the `neighbor_axis` unpacking that existed only to serve the mutuality check. What remains is the self-gluing rejection, which is load bearing: xgcm accepts a face glued to itself, nothing further downstream notices, and a section is then traced as though the seam were absent. The docstring now describes a function that enforces exactly one rule and records why that one is sectionate's to catch while the others are not. The test that pinned the face-label indexing bug would have passed vacuously once the existence check was gone, so it is reframed rather than dropped: it now asserts both that a legal `face = [1, 2]` grid passes and that the same grid with face 2 glued to itself is still rejected and named as face 2. That keeps a real regression guard on the surviving check comparing labels rather than positions along the face dimension. The test asserting xgcm rejects the malformed grids at construction is kept and its docstring updated -- that upstream contract is now the entire reason not to check here. Full suite: 81 passed, 0 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removed outdated comments regarding self-glued grids and clarified the purpose of the section tracing function.
1 participant
Addresses the wording, documentation and small-fix comments from the review of MOM6-community#47 (MOM6-community#47, review
4899521952). Targetstopology-driven-neighborsso it merges back into that PR.The two behaviour-changing comments are separate PRs, as asked: #10 (segment curves) and #11 (dropping repeated corners).
Changes
examples/load_example_ECCO_grid.py— Zenodo record bumped to v3.0.0 (10.5281/zenodo.21479854); prose no longer restates a version DOI, citing only the version-independent concept DOI and pointing atZENODO_RECORD_IDas the single place the version lives.That bump was more than a number: v3.0.0 concatenates each collection along
time, so the twelve monthly volume-flux entries collapse to one 2010 file. The new file was downloaded and verified — MD5 matches the publisheda957bd1d…, sameUVELMASS/VVELMASSon the same dims with the same twelve timestamps, andload_ECCO_MOC_gridbuilds identicalutr/vtr. The geometry file's checksum is unchanged.sectionate/gridutils.py— comment and docstring only in the first commit (verified by AST comparison ignoring comments and docstrings: executable code identical). Jargon removed, the single-tile branch marked, "grid cut" defined, and LLC90-specific remarks reframed as examples with their measured numbers kept as LLC90 observations.sectionate/transports.py—_in_velocity_rangegained a docstring explaining what it decides and why, plus an explicitUbranch and anelsethat raises rather than silently treating anything unexpected asU.sectionate/section.py—_check_supported_topologyno longer references the never-merged xgcm#707 representation; it is a generic check stated in sectionate's own terms.CLAUDE.md— the four requested edits, plus one dangling reference the removals left behind.Later commits: corrections found by an independent review
The full detail is in a comment on this PR. In brief:
_check_supported_topologydocstring claimed xgcm enforces neither face existence nor mutuality. It enforces both atGrid.__init__(KeyErrorandTypeErrorrespectively), so two of the three branches are unreachable on any legitimately-built grid. Only the self-gluing rejection is load-bearing; the docstring now says so and the two tests that corrupted the private_face_connectionsdict to reach the others are replaced by one asserting xgcm's own construction-time rejection.faces = set(range(n_faces))wrongly rejected a legal grid labelledface = [1, 2]. It now reads the labels themselves. Pinned by a new test.padded_transportsclaimed independence from the face-connection halo. That is backwards —_module_padisxgcm.padding.padand_OuterTopologybuilds its node graph through it. It is independent of the vector pad; the docstring now says which, and states what it is not independent of. Also fixed: "three things" followed by four, and a cut lip's zero justified as a wall's true transport when it is the conservative choice.Tests
81 passed, 0 skips, indocs_env_sectionate_pr47-review-docs(xgcm 0.10.1, example data present).Notebooks are re-executed once on the integrated branch, not here.
🤖 Generated with Claude Code