feat[next]: staggered fields#2667
Conversation
38714de to
8dd276b
Compare
e8cd8b4 to
14427fb
Compare
b6b4771 to
674d4fa
Compare
14427fb to
c581df8
Compare
674d4fa to
874f468
Compare
c581df8 to
7258b42
Compare
3ed2d5e to
df11778
Compare
havogt
left a comment
There was a problem hiding this comment.
GLM 5.2: Review of the staggered-fields feature, run against the current head (df11778e2). Most of the earlier round (mixing restriction, _collect_dimensions_from_params block sizes, extract_connectivity_args device guard, field(Off[idx]) removal) is resolved in the rebased tree — thanks.
One item that was reported earlier and marked resolved (connectivity_for_cartesian_shift's bare assert half == 0.5, tehrengruber-ai "Done (c7ad10812)") does not appear at this head, so the fix looks to have been lost during the rebase. Details in the inline comment at common.py:1468.
Remaining items are mostly ADR completeness, error-message formatting, and a few test gaps for the new code paths. No other correctness issues found — the half-integer offset arithmetic is correct for all sign/direction combinations I checked, and the dace/gtfn/embedded backends handle relocation consistently.
| "staggered" flag to `Dimension` would have required threading it through every one | ||
| of these representations and every place that constructs, compares, or lowers a | ||
| dimension — a large, invasive change touching the frontend, the IR, and all | ||
| backends. Carrying the marker in the name instead means it round-trips through all |
There was a problem hiding this comment.
GLM 5.2: The "round-trips through all representations for free" claim isn't quite accurate — backends actively handle the prefix:
- gtfn (
itir_to_gtfn_ir.py:_add_staggered_aliases, ~line 200) turns every staggered tag into a C++aliasof its base dimension'sTagDefinition, andvisit_AxisLiteral(~line 401) emitsnode.type.dim.valueinstead ofnode.value. - nanobind (
nanobind.py:212) usescommon.as_non_staggered(dim).valueso the generated binding references the base dimension name.
A contributor adding a new backend needs to know the prefix must be handled at code-generation boundaries. Suggest adding a short "Backend handling" subsection.
There was a problem hiding this comment.
@tehrengruber-ai Let's remove the common.as_non_staggered(dim) in the bindings unless there is a reason this is needed.
There was a problem hiding this comment.
Addressed in 544232d: softened the 'round-trips for free' wording and added a 'gtfn backend' section describing the alias handling.
havogt
left a comment
There was a problem hiding this comment.
Please check my questions. No need to do anything if you think current state makes sense.
| @@ -0,0 +1,101 @@ | |||
| --- | |||
There was a problem hiding this comment.
Index 24 and 25 is taken in the meantime.
|
|
||
| - [0005 - Extending Iterator IR](0005-Extending_Iterator_IR.md) | ||
| - [0023 - Fingerprinting](0023-Fingerprinting.md) | ||
| - [0024 - Staggered Dimensions](0024-Staggered_Dimensions.md) |
There was a problem hiding this comment.
| - [0024 - Staggered Dimensions](0024-Staggered_Dimensions.md) | |
| - [0026 - Staggered Dimensions](0026-Staggered_Dimensions.md) |
| case foast.BinOp( | ||
| op=dialect_ast_enums.BinaryOperator.ADD | dialect_ast_enums.BinaryOperator.SUB, | ||
| left=foast.Name() as dim_name, | ||
| left=foast.LocatedNode(type=ts.DimensionType(dim=common.Dimension() as dim)), |
| "Domain", | ||
| "unit_range", | ||
| "UnitRange", | ||
| "is_staggered", |
There was a problem hiding this comment.
Should is_staggered and as_non_staggered be part of the public API? Do you have a use-case?
A staggered dimension is a dimension sitting at the half-integer positions of a base dimension. For example in a cell centered 2d cartesian grid with cells located at
I,Jthe edges are located atI - ½,JandI,J - ½. Differentiating between staggered and non-staggered dimensions avoids accidental errors when fields defined on different entities are combined and improves readability for users by staying close to the mathematical formulations.Let
i_fieldfor simplicity be a 1d field of cell values defined onIandIHalfthe corresponding staggered dimension of edges between those cells theni_field(IHalf + 0.5)→ maps edges to the cell value above(i−½)+½ = ii_field(IHalf − 0.5)→ maps edges to the cell value below(i−½)−½ = i−1Similary, let
ihalf_fieldbe defined onI - ½thenihalf_field(I + 0.5)→ maps cells to the edge abovei+½ihalf_field(I − 0.5)→ maps cells to the edge belowi−½Storage
ihalf_field.ndarray[idx]holds the value for logical indexi = domain.start + idx, which is interpreted as sitting ati − ½. In the example above,IHalf(0)therefore sits at0 − ½ = −½, i.e. it is the index of the edge just below the cellI(0). The memory layout is identical to any normal field; the "half" is purely how the index is interpreted geometrically.Replaces #2339