Skip to content

refactor[next]: identify a connectivity by its local dimension, not by its tag - #78

Draft
havogt wants to merge 1 commit into
offset_type_namefrom
fieldoffset-dims
Draft

havogt wants to merge 1 commit into
offset_type_namefrom
fieldoffset-dims

Conversation

@havogt

@havogt havogt commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Stacked on GridTools#2730 (refactor[next]: carry the connectivity name in OffsetType``). Opened here in the fork so the diff is only the incremental commit; base is offset_type_name, GridTools#2730's own branch, into which `main` has been merged (`e9f588886`). Not for upstream review as-is — GridTools#2730 lands first.

Why

A FieldOffset's tag, its declared local dimension, the connectivity's own local dimension and the offset-provider key all have to be the same string. This is not a FieldOffset constraint — a sparse field argument with no FieldOffset anywhere fails identically:

Local = Dimension("Local", kind=DimensionKind.LOCAL)

@field_operator
def fo(s: gtx.Field[gtx.Dims[Vertex, Local], gtx.float64]) -> ...:
    return neighbor_sum(s, axis=Local)

fo(s, out=out, offset_provider={"V2E": conn})
# KeyError: "Offset 'Local' not found in offset provider."   embedded AND gtfn

Eleven sites hold a local Dimension, need its ConnectivityType (max_neighbors, skip_value), and recover it by using dim.value as an offset-provider key. embedded/nd_array_field.py says so in a comment (# assumes offset and local dimension have same name, since GridTools#1441); unroll_reduce acquired it in GridTools#2267 when it stopped reading the OffsetLiteral off the IR and started reading arg.type.offset_type.value.

GridTools#2730 fixes the leg where the source-level identifier became the tag. This fixes the leg where the local dimension becomes the tag.

What

  • common.get_offset_by_neighbor_dim — find the element whose neighbor_dim is the given dimension; raise naming the tags if ambiguous.
  • embedded._List and SparseListIterator carry the Dimension rather than a tag, so _List.__gt_type__ needs no lookup at all.
  • dace reads conn_type.neighbor_dim where it previously rebuilt a Dimension from the tag.
  • transform_utils._check_offset_declarations, called from past_to_gtir beside _deduce_grid_type, checks each FieldOffset's source/target against the connectivity supplied for it. Required, not extra scope: the lookup change removes the accidental KeyError that used to catch a declaration disagreeing with its connectivity, and gtfn would otherwise run such a program silently.

itir_to_gtfn_ir already emitted both C++ tag structs when the names differed (GridTools#1789, db5325bf0); nothing else was prepared.

Notes for review

  • Four of the eleven sites were not findable by grepping get_offset( — they reach the provider via a raw dict index, a SparseTag, or a fabricated Dimension. The new tests found them.
  • test_unroll_reduce.py needed updating, and how is itself evidence: dummy_connectivity_type built every connectivity with neighbor dim dummy_neighbor while registering it under "Dim"/"Dim2". The dimension the ListType carried and the dimension the connectivity carried were already inconsistent in the fixtures and nothing noticed, because only the key was ever read.
  • New failure mode: two connectivities sharing a local dimension now raise instead of silently resolving to whichever is named like it. icon4py has 15 local dims, each belonging to exactly one offset — no ambiguity there.
  • Embedded field-operator calls do not go through past_to_gtir, so the declaration check does not fire there; the pre-existing "Field can not be reduced as it doesn't have dimension" error stands. Both paths fail, only the wording differs.
  • Could be split into two commits (lookup / declaration check) if preferred.

Tests

New tests/next_tests/integration_tests/feature_tests/ffront_tests/test_offset_naming.py — 36 passing across embedded / roundtrip / gtfn / dace × simple_mesh / skip_value_mesh.

mypy src/ clean (361 files), ruff check, ruff format --check, tach check clean. unit_tests 2135 passed; multi_feature_tests 532 passed; reductions + external_local_field + import_from_mod + scan + offset_naming 517 passed.

https://claude.ai/code/session_01Mg1pdvaBDqG1meMcWjvWH7

…y its tag

Eleven sites held a local `Dimension` and needed its `ConnectivityType`
(`max_neighbors`, `skip_value`), and each recovered it by using `dim.value` as an
offset-provider key. That is what forces a `FieldOffset`'s tag, its declared local
dimension, the connectivity's own local dimension and the provider key to be one
string. It is not a `FieldOffset` constraint: a sparse field argument reduced with
`neighbor_sum(s, axis=Local)` and no `FieldOffset` anywhere fails the same way.

`common.get_offset_by_neighbor_dim` finds the element whose `neighbor_dim` is the
given dimension, raising if it is ambiguous. `embedded._List` and
`SparseListIterator` now carry the dimension rather than a tag, so `_List.__gt_type__`
needs no lookup at all; the dace lowering reads `conn_type.neighbor_dim` where it
previously rebuilt a dimension from the tag.

`transform_utils._check_offset_declarations`, called from `past_to_gtir` beside
`_deduce_grid_type`, checks each `FieldOffset`'s `source`/`target` against the
connectivity supplied under its name. This is required rather than incidental: the
lookup change removes the accidental `KeyError` that used to catch a declaration
disagreeing with its connectivity, and gtfn would otherwise run such a program
silently.

`itir_to_gtfn_ir` already emitted both C++ tag structs when the names differed
(GridTools#1789); nothing else was prepared.

Claude-Session: https://claude.ai/code/session_01Mg1pdvaBDqG1meMcWjvWH7
@havogt
havogt changed the base branch from offset_type_name-rebased to offset_type_name September 4, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant