feat[next]: make a concrete Dimension a class and its instances the indices - #2844
feat[next]: make a concrete Dimension a class and its instances the indices#2844egparedes wants to merge 1 commit into
Conversation
|
Review notes (kept out of the description so they don't land in the squashed commit) Part of a stack: #2843 (merged) → #2844 → #2845. This PR now targets Scope note, since the boundary moved: this PR rewrites existing declarations only to the minimal Also verified locally beyond CI: |
559366a to
5e0c4ef
Compare
5e0c4ef to
a7653b0
Compare
…ass' (#2843) `eve.type_validation` had no case for `type[X]`, so the annotation fell through to the custom-generic-type branch and validated only `isinstance(value, type)`. A DataModel field annotated `type[Foo]` therefore accepted any class at all, including `int`: ```python class Holder(datamodels.DataModel): dim: type[common.Dimension] Holder(dim=int) # accepted before this change ``` Adds a `type[X]` case that checks the actual subclass relationship: - `type[SomeClass]` validates by `issubclass`. - `type[A | B]` validates as an OR of subclass checks. - `type[SomeAlias]` resolves a nested PEP 695 alias first — whole-annotation alias resolution runs once at the top of the factory and does not reach inside `type[...]`. - `type[T]` honours `T.__bound__` when there is one, mirroring the plain-`TypeVar` branch. - Bare `type` / `typing.Type`, `type[Any]` and unbound TypeVars keep the loose "is a class" check, which is all they can mean. - `type[SomeProtocol]` also keeps the loose check. `issubclass` is not generally usable with protocols: it is rejected outright unless the protocol is `@runtime_checkable`, and rejected again for `@runtime_checkable` protocols that have non-method members. A strict check would raise `TypeError` for *every* value, making the field unusable. Any other shape falls back to that same loose check rather than raising, so no annotation that validated before can start failing — neither at class-creation time nor at validation time. One existing field becomes strictly validated: `ts.DeferredType.constraint`, the only `type[...]` annotation on any `DataModel` in the tree. It was already correct — every in-tree `constraint=` call site passes a `ts.*Type` class, `None`, or a tuple of them. Also fixes documentation drift found alongside: - `docs/development/ADRs/next/README.md` linked two files that do not exist (the 0026 entry pointed at `0024-Staggered_Dimensions.md`, the 0012 entry at `0011-_GridTools_Cpp_OTF.md`), listed ADR 0018 twice, and omitted six ADRs entirely: 0014, 0019, 0020, 0021, 0024, 0025. - `common.connectivity_for_cartesian_shift` cited ADR 0024 (*Compilation Runners*) for the staggered-index convention; that convention is ADR 0026. - Records the mypy plugin's undocumented `*Dim` naming requirement (`fullname.endswith("Dim")`), which our own QuickstartGuide violated. Prerequisite for #2844, which annotates every dimension-typed DataModel field as `type[common.Dimension]` — including `CartesianConnectivity.domain_dim: type[DomainDimT]`, which relies on the TypeVar bound being honoured.
b81d4d5 to
fefb8e2
Compare
ab5e8d5 to
9d4ff7d
Compare
9d4ff7d to
9c72ef2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Some updated tests contain non-asserted comparisons (effectively no-ops), and one conversion helper change can misclassify non-dimension type[T] annotations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR implements the “dimensions as types” design for gt4py.next: concrete dimensions become classes (DimensionIndex subclasses) and indices become instances (IDim(0)), enabling plugin-free static typing (mypy/pyright) and removing the dimension-handling portion of the mypy plugin. It updates the next runtime/type system, downstream tests, examples, and user docs accordingly, and records the decision in ADR 0028.
Changes:
- Introduce/propagate the new dimension model (
DimensionIndexclasses,.tagfor dimension names,dimension()factory for programmatic construction) across runtime, typing, codegen, and transformations. - Update tests/examples/docs to the new declarations and indexing (
NamedIndex→Dim(value)), and add ADR 0028 + CHANGELOG entry. - Simplify
mypy_plugin.pyto only blur scalar precision, keeping a deprecated alias for compatibility.
File summaries
| File | Description |
|---|---|
| typing_tests/test_next.yaml | Switch typing snippets from gtx.Dimension(...) values to class-style DimensionIndex declarations. |
| tests/next_tests/unit_tests/type_system_tests/test_type_translation.py | Update dimension construction in type translation tests to gtx.dimension(...). |
| tests/next_tests/unit_tests/type_system_tests/test_type_info.py | Update dimension fixtures/imports to new dimension(...) factory. |
| tests/next_tests/unit_tests/test_utils.py | Update dimension construction for fingerprinting-related tests. |
| tests/next_tests/unit_tests/test_field_utils.py | Update field constructor test to use common.dimension(...). |
| tests/next_tests/unit_tests/test_custom_layout_allocators.py | Replace NamedIndex usage with dimension-typed index instances (Dim(n)) and update aligned-index typing. |
| tests/next_tests/unit_tests/test_constructors.py | Update dimension declarations and aligned index construction to new index instances. |
| tests/next_tests/unit_tests/test_common.py | Update dimension creation and add extensive tests for class-style dimensions + pickling/fingerprinting behavior. |
| tests/next_tests/unit_tests/program_processor_tests/runners_tests/dace_tests/transformation_tests/test_map_promoter.py | Adjust DaCe map variable construction to new dimension(...) factory. |
| tests/next_tests/unit_tests/program_processor_tests/codegens_tests/gtfn_tests/test_gtfn_module.py | Update test dimension construction to gtx.dimension(...). |
| tests/next_tests/unit_tests/otf_tests/test_runners.py | Update dimensions used in OTF connectivity shipping tests. |
| tests/next_tests/unit_tests/otf_tests/test_compiled_program.py | Update placeholder dimension to gtx.dimension(...). |
| tests/next_tests/unit_tests/otf_tests/compilation_tests/build_systems_tests/conftest.py | Update dimension construction in generated interface types. |
| tests/next_tests/unit_tests/otf_tests/binding_tests/test_cpp_interface.py | Update dimension construction in C++ interface tests. |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_unroll_reduce.py | Update local-offset dimension construction to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_prune_empty_concat_where.py | Update dimension vars and accessed-domain typing to new dimension types. |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_prune_casts.py | Update dimension construction in cast pruning tests. |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_inline_scalar.py | Update placeholder dimension to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_global_tmps.py | Update dimension fixtures to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_fuse_as_fieldop.py | Update dimension fixtures and local offset type creation. |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_expand_tuple_maps.py | Update dimension fixture to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_domain_inference.py | Update dimension fixtures to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_dead_code_elimination.py | Update placeholder dimension to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_cse.py | Update offset-provider type fixture to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_concat_where_transform_to_as_fieldop.py | Update dimension fixtures to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_concat_where_expand_tuple_args.py | Update dimension fixture to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_concat_where_canonicalize_domain_args.py | Update dimension fixture to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_collapse_tuple.py | Update dimension fixture to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/test_runtime_domain.py | Update runtime-domain test dimensions to gtx.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/test_inline_dynamic_shifts.py | Update dimension fixture to gtx.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/test_embedded_internals.py | Update embedded context dimension creation to common.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/test_embedded_field_with_list.py | Update unstructured dimension fixtures to gtx.dimension(...). |
| tests/next_tests/unit_tests/iterator_tests/ir_utils_tests/test_domain_utils.py | Update fixtures and typing in domain-utils tests to new dimension/index types. |
| tests/next_tests/unit_tests/ffront_tests/test_type_deduction.py | Update many frontend test dimensions to dimension(...) and imports accordingly. |
| tests/next_tests/unit_tests/ffront_tests/test_stages.py | Update stage test dimension to gtx.dimension(...). |
| tests/next_tests/unit_tests/ffront_tests/test_func_to_foast.py | Update placeholder/test dimensions to gtx.dimension(...) and axis-literal tag usage. |
| tests/next_tests/unit_tests/ffront_tests/test_func_to_foast_error_line_number.py | Update placeholder dimension to gtx.dimension(...). |
| tests/next_tests/unit_tests/ffront_tests/test_foast_to_gtir.py | Update dimensions and axis literal construction to use .tag. |
| tests/next_tests/unit_tests/ffront_tests/test_diagnostic_messages.py | Update diagnostic test dimension to gtx.dimension(...). |
| tests/next_tests/unit_tests/ffront_tests/test_decorator_domain_deduction.py | Update grid-type deduction tests to gtx.dimension(...). |
| tests/next_tests/unit_tests/embedded_tests/test_nd_array_field.py | Update embedded/nd-array tests to dimension(...) and index instances (Dim(n)). |
| tests/next_tests/unit_tests/embedded_tests/test_context.py | Update NamedRange dimensions to common.dimension(...). |
| tests/next_tests/unit_tests/embedded_tests/test_common.py | Update embedded common tests to use new index instances and dimension(...). |
| tests/next_tests/unit_tests/embedded_tests/test_basic_program.py | Update basic embedded program test dimension. |
| tests/next_tests/unit_tests/conftest.py | Update dummy connectivity dims to new gtx.dimension(...) constructor. |
| tests/next_tests/toy_connectivity.py | Update toy connectivity dims to gtx.dimension(...). |
| tests/next_tests/regression_tests/ffront_tests/test_offset_dimensions_names.py | Update regression dims to gtx.dimension(...). |
| tests/next_tests/regression_tests/embedded_tests/test_domain_pickle.py | Update regression dims to common.dimension(...). |
| tests/next_tests/integration_tests/multi_feature_tests/iterator_tests/test_temporaries.py | Update integration dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/multi_feature_tests/iterator_tests/test_if_stmt.py | Update integration dim to gtx.dimension(...). |
| tests/next_tests/integration_tests/multi_feature_tests/iterator_tests/test_fvm_nabla.py | Update integration dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/multi_feature_tests/iterator_tests/test_anton_toy.py | Update integration dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/multi_feature_tests/fvm_nabla_setup.py | Update setup dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/multi_feature_tests/ffront_tests/test_icon_like_scan.py | Update scan test dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/feature_tests/iterator_tests/test_tuple.py | Update feature test dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/feature_tests/iterator_tests/test_strided_offset_provider.py | Update feature test dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/feature_tests/iterator_tests/test_program.py | Update feature test dim to gtx.dimension(...). |
| tests/next_tests/integration_tests/feature_tests/iterator_tests/test_implicit_fencil.py | Update feature test dim to gtx.dimension(...). |
| tests/next_tests/integration_tests/feature_tests/iterator_tests/test_conditional.py | Update feature test dim to gtx.dimension(...). |
| tests/next_tests/integration_tests/feature_tests/iterator_tests/test_builtins.py | Update builtins feature test dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/feature_tests/instrumentation_tests/test_hooks.py | Update instrumentation test dim to gtx.dimension(...). |
| tests/next_tests/integration_tests/feature_tests/ffront_tests/test_foast_pretty_printer.py | Update pretty-printer tests to new dimension construction/imports. |
| tests/next_tests/integration_tests/feature_tests/dace_tests/test_write_back_buffer_elimination_lowering.py | Update DaCe feature test dims to gtx.dimension(...). |
| tests/next_tests/integration_tests/cases_utils.py | Update shared test dims/connectivities to gtx.dimension(...). |
| tests/next_tests/fixtures/past_common.py | Update fixture dims to gtx.dimension(...). |
| tests/next_tests/benchmarks/benchmark_program_call.py | Update benchmark dims to gtx.dimension(...). |
| tests/next_tests/artifacts/custom_named_collections.py | Update artifact dimensions/imports to dimension(...). |
| src/gt4py/next/type_system/type_translation.py | Update dimension type checks from Dimension to DimensionMeta and cast appropriately. |
| src/gt4py/next/type_system/type_specifications.py | Switch field-type stringification from dim.value to dim.tag and clarify dimension-level vs DSL-level types. |
| src/gt4py/next/type_system/type_info.py | Update doctests and error strings to use .tag and new dimension(...) factory. |
| src/gt4py/next/type_system/mypy_plugin.py | Remove dimension substitution hooks; keep scalar-precision blurring with a compatibility alias. |
| src/gt4py/next/program_processors/runners/roundtrip.py | Update generated source to use gtx.dimension(...) for axis literals. |
| src/gt4py/next/program_processors/runners/dace/transformations/map_orderer.py | Update dimension instance checks to DimensionMeta. |
| src/gt4py/next/program_processors/runners/dace/transformations/loop_blocking.py | Update blocking-parameter handling to recognize DimensionMeta. |
| src/gt4py/next/program_processors/runners/dace/sdfg_args.py | Use .tag for symbol naming and dimension(...) for axis construction. |
| src/gt4py/next/program_processors/runners/dace/lowering/gtir_to_sdfg.py | Replace .value offset lookups with .tag for offset-provider interactions. |
| src/gt4py/next/program_processors/runners/dace/lowering/gtir_to_sdfg_utils.py | Update map-variable naming to use .tag. |
| src/gt4py/next/program_processors/runners/dace/lowering/gtir_to_sdfg_scan.py | Use .tag for offset-provider lookups in scan lowering. |
| src/gt4py/next/program_processors/runners/dace/lowering/gtir_to_sdfg_primitives.py | Use .tag for offset-provider lookups when lowering list-typed branches. |
| src/gt4py/next/program_processors/runners/dace/lowering/gtir_to_sdfg_lambda.py | Update internal magic dims, connector formatting, and offset-provider lookups to .tag/dimension(...). |
| src/gt4py/next/program_processors/codegens/gtfn/itir_to_gtfn_ir.py | Switch tag/offset definition collection and axis literals to .tag and DimensionMeta guards. |
| src/gt4py/next/program_processors/codegens/gtfn/gtfn_module.py | Use .tag for dimension names in generated module argument processing. |
| src/gt4py/next/otf/compilation_tasks.py | Update offset-provider serialization to treat dimensions as DimensionMeta. |
| src/gt4py/next/otf/binding/nanobind.py | Switch dimension naming from .value to .tag in C++ binding specs. |
| src/gt4py/next/iterator/type_system/type_synthesizer.py | Update doctests and asserts to treat dimensions as DimensionMeta. |
| src/gt4py/next/iterator/type_system/inference.py | Build DimensionType from common.dimension(...) rather than Dimension(...) dataclass. |
| src/gt4py/next/iterator/transforms/unroll_reduce.py | Use .tag for offset-type tag extraction. |
| src/gt4py/next/iterator/transforms/replace_get_domain_range_with_constants.py | Match domain dims via .tag for constant range replacement. |
| src/gt4py/next/iterator/transforms/remove_broadcast.py | Update example axis literals to use .tag. |
| src/gt4py/next/iterator/transforms/prune_empty_concat_where.py | Update examples to common.dimension(...). |
| src/gt4py/next/iterator/transforms/pass_manager.py | Use .tag when extracting connectivity dimensions. |
| src/gt4py/next/iterator/transforms/inline_fundefs.py | Update example dimension creation to common.dimension(...). |
| src/gt4py/next/iterator/transforms/fuse_as_fieldop.py | Update example dimension creation to gtx.dimension(...). |
| src/gt4py/next/iterator/tracing.py | Treat dimension classes (DimensionMeta) as axis literals using .tag. |
| src/gt4py/next/iterator/ir_utils/misc.py | Rebuild dims from axis literals via common.dimension(...). |
| src/gt4py/next/iterator/ir_utils/ir_makers.py | Treat DimensionMeta as axis literals and build axis literals from .tag. |
| src/gt4py/next/iterator/ir_utils/domain_utils.py | Use common.dimension(...) and .tag for symbolic-domain translation. |
| src/gt4py/next/iterator/embedded.py | Update embedded execution paths to use .tag, dimension classes, and index instances. |
| src/gt4py/next/fingerprinting.py | Ensure dimension classes fingerprint by (tag, kind) rather than fully qualified name. |
| src/gt4py/next/field_utils.py | Update doctests to common.dimension(...). |
| src/gt4py/next/ffront/type_info.py | Update doctests to common.dimension(...) and new repr output. |
| src/gt4py/next/ffront/transform_utils.py | Update grid-type deduction to treat dimensions as DimensionMeta. |
| src/gt4py/next/ffront/past_to_itir.py | Update closure-var filtering and error messaging to .tag and DimensionMeta. |
| src/gt4py/next/ffront/func_to_past.py | Update examples to gtx.dimension(...). |
| src/gt4py/next/ffront/func_to_foast.py | Update examples to gtx.dimension(...). |
| src/gt4py/next/ffront/foast_to_past.py | Update examples to gtx.dimension(...). |
| src/gt4py/next/ffront/foast_to_gtir.py | Update axis literal emission and pattern matching to .tag and DimensionMeta. |
| src/gt4py/next/ffront/foast_pretty_printer.py | Update examples to dimension(...). |
| src/gt4py/next/ffront/foast_passes/type_deduction.py | Update imports/types and error strings from .value to .tag. |
| src/gt4py/next/ffront/field_operator_ast.py | Rename dimension type variable for clarity in the AST type definitions. |
| src/gt4py/next/ffront/fbuiltins.py | Update builtins type conversion to account for PEP 695 alias resolution and new dimension root (DimensionIndex). |
| src/gt4py/next/ffront/decorator.py | Update doctest dimension declaration to gtx.dimension(...). |
| src/gt4py/next/embedded/operators.py | Update scan tuple-index handling to use DimensionIndex instances. |
| src/gt4py/next/embedded/nd_array_field.py | Update domain/dimension checks to DimensionMeta and replace NamedIndex handling with index instances. |
| src/gt4py/next/embedded/common.py | Update named-index/range plumbing to handle DimensionIndex instances and .tag in messages. |
| src/gt4py/next/custom_layout_allocators.py | Update allocator API typing (aligned_index) from NamedIndex to DimensionIndex. |
| src/gt4py/next/constructors.py | Update constructors to accept aligned index instances and treat dimension classes as DimensionMeta. |
| src/gt4py/next/init.py | Export DimensionIndex and dimension from the top-level gt4py.next API. |
| examples/lap_cartesian_vs_next.ipynb | Migrate example notebook to class-style dimensions. |
| docs/user/next/workshop/slides/slides_4.ipynb | Migrate workshop slide snippet to class-style dimension. |
| docs/user/next/workshop/slides/slides_3.ipynb | Migrate workshop slide snippets to class-style dimensions. |
| docs/user/next/workshop/slides/slides_2.ipynb | Migrate workshop slide snippets (including LOCAL dims with pinned tag) to class-style. |
| docs/user/next/workshop/slides/slides_1.ipynb | Migrate workshop slide snippet to class-style dimensions. |
| docs/user/next/workshop/exercises/helpers.py | Migrate workshop helper dimensions and offsets to class-style declarations with pinned tags. |
| docs/user/next/workshop/exercises/1_simple_addition.ipynb | Migrate exercise notebook to class-style dimensions. |
| docs/user/next/workshop/exercises/1_simple_addition_solution.ipynb | Migrate exercise solution notebook to class-style dimensions. |
| docs/user/next/QuickstartGuide.md | Update user guide code cells to class-style dimensions and pinned tags. |
| docs/development/ADRs/next/README.md | Add ADR 0028 to the next ADR index. |
| docs/development/ADRs/next/0028-Dimensions_As_Types.md | New ADR documenting the dimensions-as-types decision and tradeoffs. |
| CHANGELOG.md | Document the breaking changes, new APIs, and migration guidance for dimensions-as-types. |
Review details
Suppressed comments (1)
tests/next_tests/unit_tests/test_common.py:458
- These comparisons are not asserted, so the test will pass even if
Domain.popreturns the wrong result.
- Files reviewed: 131/131 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if t is common.Field: | ||
| return ts.FieldType | ||
| elif t is common.Dimension: | ||
| elif t is common.DimensionIndex or get_origin(t) is type: | ||
| return ts.DimensionType | ||
| elif t is FieldOffset: |
There was a problem hiding this comment.
Fixed. The branch now requires type[X] with X a DimensionIndex subclass, so type[int] falls through (and a bare type is still handled by the ts.ConstructorType branch below). In practice _type_conversion_helper only ever sees the annotations of the builtin signatures in this module, where the only type[...] is common.Dimension, but the narrower test costs nothing.
| def test_domain_dim_index(): | ||
| dims = [Dimension("X"), Dimension("Y"), Dimension("Z")] | ||
| dims = [dimension("X"), dimension("Y"), dimension("Z")] | ||
| ranges = [UnitRange(0, 1), UnitRange(0, 1), UnitRange(0, 1)] | ||
| domain = Domain(dims=dims, ranges=ranges) | ||
|
|
||
| domain.dim_index(Dimension("Y")) == 1 | ||
| domain.dim_index(dimension("Y")) == 1 | ||
|
|
||
| domain.dim_index(Dimension("Foo")) == None | ||
| domain.dim_index(dimension("Foo")) == None |
There was a problem hiding this comment.
Fixed, and the same defect two functions down in test_domain_pop (three more bare comparisons) — both predate this PR, which only rewrote the Dimension(...) calls on those lines. dim_index returns None for a missing dimension, so the second one is now is None. All four assertions pass.
…ndices A concrete dimension is now declared as a class, and an index along it is an instance of that class -- the shape `enum.Enum` uses, where the class is the collection and the instances are its members: ```python class IDim(gtx.DimensionIndex): ... class KDim(gtx.DimensionIndex, kind=gtx.DimensionKind.VERTICAL): ... IDim # the dimension -- annotated `gtx.Dimension` IDim(0) # an index into it -- annotated `IDim` ``` so `gtx.Field[gtx.Dims[IDim], gtx.float64]` is a valid annotation for any PEP 484 type checker, with no gt4py mypy plugin. `DimensionMeta` carries the API that belongs to the dimension itself (`I + 1`, `I > 5`, `I == 5`, `repr`, equality, hashing); binary operators on a class object dispatch through the metaclass, so that is the only place they can live. This drops the dimension half of `mypy_plugin.py`, which substituted at most four distinct placeholders per run (`_DimA`..`_DimD`, then `_AnyDim` for everything after), made `TypeVar`s over dimensions impossible, and served only mypy. Because `IDim(0)` is now ordinary instantiation, the whole apparatus that a foreign return type required is gone: no `__new__` returning a non-instance, no duplicated overloads on both `DimensionMeta.__call__` and `__new__`, and no `# type: ignore[misc]` anywhere on the instantiation path. mypy and pyright agree natively -- pinned by the typing tests in #2845. Indices also carry their dimension in the type, so mixing them is a static error; `common.NamedIndex` is deleted. Naming: the dimension's name is `.tag` (typed `common.Tag`, which already existed), and `.value` keeps its meaning as the index position. The reverse split does not type-check at all -- an instance attribute cannot shadow a `ClassVar` -- and this direction leaves every index expression, downstream included, untouched. `.dim` survives as a property returning `type(self)`. `common.Dimension` is a PEP 695 alias for `type[DimensionIndex]`, re-exported as `gtx.Dimension`, and `common.dimension(tag, kind)` is the programmatic constructor for the IR boundaries that rebuild a dimension from its tag. The PEP 695 spelling is what makes the deprecated `gtx.Dimension("I")` raise rather than silently misbehave. A plain `TypeAlias` for `type[X]` is a `types.GenericAlias`, and calling one forwards to its `__origin__` while discarding the arguments -- so `Dimension("I")` would evaluate to `type("I")`, i.e. `str`, with no error. Its cost is that `get_origin()` of such an alias is `None`, so a site dispatching on an annotation's shape must resolve it first; exactly one in-tree site needed that, `ffront.fbuiltins._type_conversion_helper`, via the `xtyping.resolve_annotation` helper added in #2841. A `TYPE_CHECKING`-split callable shim was also tried and rejected: `eve.datamodels` resolves annotations at run time, so a `Dimension` field would see the shim rather than a type. Reading `.value` on a dimension *class* would otherwise return the `__slots__` member descriptor rather than raising, and the nonsense value only surfaces much later as a missing offset-provider key or an `AxisLiteral` validation failure. A metaclass property makes it a loud `AttributeError` pointing at `.tag`; instance access is unaffected, since a metaclass attribute is not on an instance's lookup path. Also: pickling is registered through `copyreg` because `pickle.Pickler.save` routes anything whose type subclasses `type` to `save_global` before consulting `__reduce_ex__`, and `fingerprinting.py` gets a `DimensionMeta` deconstructor keyed on `(tag, kind)` so a dimension is not fingerprinted by qualified name. Behaviour change: `repr()` of a dimension is now `I[horizontal]`; `str()` is unchanged, so error messages are byte-identical. Design record: ADR 0028, added here. Implements the `shared/dimensions-as-types` proposal (gt4py_knowledge#27, @havogt) and closes the static-typing gap reported in #2503. Deliberately not here: a `DimensionBase` root above the user-declarable class, deferred until the requirements of non-user-declarable dimensions such as `Staggered[D]` are known; ICON4Py migration, which needs a note for `.tag` and for the removal of `NamedIndex`.
9c72ef2 to
26016ac
Compare
|
cscs-ci run default |
1 similar comment
|
cscs-ci run default |
|
Why I don't think it is this PR:
The parent commit |
|
cscs-ci run default |
| # NOTE: `NamedRange` is a namedtuple but an index is a `DimensionIndex` | ||
| # instance, so the payload has to be selected rather than unpacked. | ||
| idx = ( | ||
| named_idx.unit_range | ||
| if isinstance(named_idx, common.NamedRange) | ||
| else named_idx.value | ||
| ) |
There was a problem hiding this comment.
Wondering if we should add __iter__ to DimensionIndex which unpacks to type(self), value?
Otherwise I would add a TODO here to cleanup this NamedRange/DimensionIndex abstraction that broke here.
| #: ``dimension(tag, kind)`` constructor resolves to an already-declared class | ||
| #: rather than shadowing it, and unpickling restores object identity rather than an | ||
| #: equal-but-distinct class. | ||
| _DIMENSION_REGISTRY: dict[tuple[Tag, DimensionKind], Dimension] = {} |
There was a problem hiding this comment.
This is ugly. Did you compare it to dropping comparison by identity?
| type Dimension = type[DimensionIndex] | ||
|
|
||
|
|
||
| def dimension(tag: Tag, kind: DimensionKind = DimensionKind.HORIZONTAL) -> Dimension: |
There was a problem hiding this comment.
What's the preferred way for a user? explicit inheritance or calling this function?
|
|
||
| Example: | ||
| >>> I = common.Dimension("I") | ||
| >>> I = common.dimension("I") |
There was a problem hiding this comment.
see question later on which method is preferred, if there is an answer, we should make it consistent in gt4py?
| # NOTE: an index is a `DimensionIndex` instance, not a 2-tuple, so its parts are | ||
| # read rather than unpacked. |
There was a problem hiding this comment.
drop these comments everywhere
| @property | ||
| def value(cls) -> NoReturn: | ||
| """ | ||
| Reject `SomeDim.value`, which used to be the dimension's name and is now `tag`. | ||
|
|
||
| def __sub__(self, offset: int | float) -> Connectivity: | ||
| return self + (-offset) | ||
| Without this, the read silently returns the `value` slot descriptor of the *instance* | ||
| attribute rather than raising, and the nonsense value only surfaces much later -- as a | ||
| missing offset-provider key, or an `AxisLiteral` validation failure. Instance access | ||
| (`SomeDim(0).value`) goes through the slot and is unaffected: a metaclass attribute is | ||
| not on an instance's lookup path. | ||
| """ | ||
| raise AttributeError( | ||
| f"'{cls.tag}' is a dimension and has no 'value': its name is '.tag', and an" | ||
| f" *index* into it -- '{cls.tag}(0)' -- is what has '.value'." | ||
| ) |
There was a problem hiding this comment.
I'd drop it already. I think we can deal with the migration without it?
| class _DimA(Dimension): ... | ||
| def _reduce_dimension(cls: DimensionMeta) -> tuple[Any, tuple[Tag, DimensionKind]]: | ||
| """ | ||
| Pickle a dimension class by `(tag, kind)` rather than by module reference. |
There was a problem hiding this comment.
Can we add info if this is the long term strategy or if we would like to change that.
| # NOTE: `==`, not `is`: dimension classes compare by `(tag, kind)` through the | ||
| # metaclass, so two same-tagged classes are the same dimension and their indices | ||
| # must compare equal. `is` here would make that comparison nominal. | ||
| return type(self) == type(other) and self.value == other.value # noqa: E721 [type-comparison] |
There was a problem hiding this comment.
since we have the registry, can't we do is?
| #: NOTE: a PEP 695 `type` statement, not a plain `TypeAlias`, so that the deprecated | ||
| #: `Dimension("I")` spelling fails loudly. A plain alias is a `types.GenericAlias`, and calling | ||
| #: one forwards to its `__origin__` while discarding the arguments -- so `Dimension("I")` would | ||
| #: evaluate to `type("I")`, i.e. `str`, with no error at all. A `TypeAliasType` is simply not | ||
| #: callable. | ||
| #: | ||
| #: The cost is that `get_origin()` of a PEP 695 alias is `None` rather than the aliased origin, | ||
| #: so a site dispatching on an annotation's shape must resolve it first (see | ||
| #: `xtyping.resolve_annotation`, and `ffront.fbuiltins._type_conversion_helper` for the one | ||
| #: in-tree site that needs it). `eve.datamodels` stores annotations *unresolved*, so this | ||
| #: applies to anything reading `__datamodel_fields__[...].type` too. See #2841 and ADR 0028. |
There was a problem hiding this comment.
Is this all needed? Shouldn't we use type instead of TypeAlias anyway?
| ## [Unreleased] | ||
|
|
||
| ### Next | ||
|
|
||
| - **Dimensions are types.** Declare a dimension as a class — `class IDim(gtx.DimensionIndex): ...` — instead of `IDim = gtx.Dimension("IDim")`. `gtx.Field[gtx.Dims[IDim], gtx.float64]` is now a valid annotation for any type checker, including pyright, with no gt4py mypy plugin. See ADR 0028. | ||
| - **Breaking:** `gtx.Dimension` is now annotation-only (a PEP 695 alias for `type[gtx.DimensionIndex]`), so `gtx.Dimension("IDim")` raises `TypeError`. Use the class statement above, or `gtx.dimension("IDim")` where a dimension has to be built programmatically from a tag. | ||
| - **Breaking:** the dimension's name moved from `dim.value` to `dim.tag`; `common.NamedIndex` is removed — an index along `IDim` is now `IDim(0)`, with the same `.dim` / `.value` accessors. | ||
| - `repr()` of a dimension is now `IDim[horizontal]`, matching what `str()` already produced. Error messages are unchanged. | ||
| - The dimension hooks are removed from `gt4py.next.type_system.mypy_plugin`; only scalar-precision blurring remains. Downstream projects that migrate to class-style dimensions no longer need the plugin for dimensions. | ||
|
|
There was a problem hiding this comment.
| ## [Unreleased] | |
| ### Next | |
| - **Dimensions are types.** Declare a dimension as a class — `class IDim(gtx.DimensionIndex): ...` — instead of `IDim = gtx.Dimension("IDim")`. `gtx.Field[gtx.Dims[IDim], gtx.float64]` is now a valid annotation for any type checker, including pyright, with no gt4py mypy plugin. See ADR 0028. | |
| - **Breaking:** `gtx.Dimension` is now annotation-only (a PEP 695 alias for `type[gtx.DimensionIndex]`), so `gtx.Dimension("IDim")` raises `TypeError`. Use the class statement above, or `gtx.dimension("IDim")` where a dimension has to be built programmatically from a tag. | |
| - **Breaking:** the dimension's name moved from `dim.value` to `dim.tag`; `common.NamedIndex` is removed — an index along `IDim` is now `IDim(0)`, with the same `.dim` / `.value` accessors. | |
| - `repr()` of a dimension is now `IDim[horizontal]`, matching what `str()` already produced. Error messages are unchanged. | |
| - The dimension hooks are removed from `gt4py.next.type_system.mypy_plugin`; only scalar-precision blurring remains. Downstream projects that migrate to class-style dimensions no longer need the plugin for dimensions. |
undo, we don't do it like this currently.
| @@ -115,6 +117,8 @@ | |||
| "is_scalar_type", | |||
| # from common | |||
There was a problem hiding this comment.
regarding the isinstance(..., DimensionMeta) vs is_dimension. Here we should export one or the other.
A concrete dimension is now a class, and an index along it an instance of that class -- the shape
enum.Enumuses, where the class is the collection and the instances are its members:so
gtx.Field[gtx.Dims[IDim], gtx.float64]is valid for any PEP 484 checker with no gt4py mypyplugin. This drops the dimension half of
mypy_plugin.py, which substituted at most four distinctplaceholders per run (
_DimA.._DimD, then_AnyDimfor everything after), madeTypeVars overdimensions impossible, and served only mypy.
DimensionMetacarries what belongs to the dimension itself (I + 1,I > 5,repr, equality,hashing) -- binary operators on a class object dispatch through the metaclass, so that is the only
place they can live. Because
IDim(0)is ordinary instantiation, nothing else is needed: no__new__returning a non-instance, no overloads duplicated on__call__and__new__, and no# type: ignoreon the instantiation path. Indices carry their dimension in the type, so mixingthem is a static error, and
common.NamedIndexis deleted --.dimand.valuekeep working, nowon the instance.
Naming: a dimension's name is
.tag(typedcommon.Tag, which already existed), and.valuekeepsits meaning as the index position. The reverse split does not type-check at all -- an instance
attribute cannot shadow a
ClassVar-- and this direction leaves every index expression, downstreamincluded, untouched.
common.Dimensionis a PEP 695 alias fortype[DimensionIndex], andcommon.dimension(tag, kind)is the programmatic constructor for the IR boundaries that rebuild a dimension from a tag. The PEP
695 spelling is what makes the removed
gtx.Dimension("I")raise rather than silently misbehave: aplain
TypeAliasfortype[X]is atypes.GenericAlias, and calling one forwards to__origin__while discarding the arguments, so it would evaluate to
strwith no error. Its cost is thatget_origin()of such an alias isNone, so a site dispatching on an annotation's shape mustresolve it first (
xtyping.resolve_annotation, added in #2841); exactly one in-tree site neededthat,
ffront.fbuiltins._type_conversion_helper.Two smaller pieces. Pickling is registered through
copyreg, becausepickle.Pickler.saveroutesanything whose type subclasses
typetosave_globalbefore consulting__reduce_ex__, andfingerprinting.pygets aDimensionMetadeconstructor keyed on(tag, kind), so a dimension isnot fingerprinted by qualified name. And reading
.valueon a dimension class raises a metaclassAttributeErrorpointing at.tag, instead of returning the__slots__member descriptor andsurfacing much later as a missing offset-provider key.
Outside
src/, existing declarations take the minimal form that keeps them working --I = gtx.Dimension("I")becomesI = gtx.dimension("I"); adopting the class spelling across thetests is #2845. Two surfaces cannot take that form and are migrated here, because they break on a
maincarrying only this PR:typing_tests/test_next.yaml, since a dimension bound to a variableis not usable as an annotation once the plugin hooks are gone, and the user docs, workshop notebooks
and
examples/, whichtest_examplesruns. Notebook code cells were migrated without touchingstored outputs, which hold recorded tracebacks that must keep naming the symbols that produced them.
Behaviour change:
repr()of a dimension is nowI[horizontal];str()is unchanged, so errormessages are byte-identical.
Design record: ADR 0028, added here. Implements the
shared/dimensions-as-typesproposal(gt4py_knowledge#27, @havogt) and closes the static-typing gap reported in #2503.
Deliberately not here: a
DimensionBaseroot above the user-declarable class, deferred until therequirements of non-user-declarable dimensions such as
Staggered[D]are known; the ICON4Pymigration note for
.tagand for the removal ofNamedIndex.