refactor(common): EdgeParams as a dataclass with explicit optional members by havogt · Pull Request #1462 · C2SM/icon4py

havogt · 2026-09-08T18:55:54Z

EdgeParams is a hand-written class whose attribute annotations do not say which members a construction path may leave unset.

What changes

EdgeParams becomes dataclass.
| None now means "some source cannot supply this"
edge_cell_distances is new, from t_grid_edges%edge_cell_length. It's not needed required by dycore/diffusion, therefore not passed in via Fortran bindings and we make it optional in this PR. Alternative could be to still pass it from Fortran, but since the Fortran array has wrong layout it would require a copy.

Why now

In #1457 it was originally added to tmx directly instead of EdgeParams, therefore we extracted this refactoring in preparation for that PR.

…mbers EdgeParams was a hand-written class whose attribute annotations did not say which members a construction path may leave unset, and were wrong where it mattered: `primal_edge_lengths` and `dual_edge_lengths` are annotated as fields but are None in every path except the two diffusion tests. - EdgeParams becomes a frozen keyword-only dataclass. The six x/y pairs the old __init__ folded into tuples are now tuple fields, so the constructor and the attributes agree; attribute names and every consumer are unchanged. - `| None` now marks exactly the members some source cannot supply: `primal_edge_lengths`, `dual_edge_lengths` and `edge_cell_distances`, the last of which ICON's `grid_init` does not pass. CellParams is tightened the same way: only `mean_cell_area` is optional. - `edge_cell_distances` is new, from t_grid_edges%edge_cell_length. The savepoint and the metrics/geometry factory fill it; the Fortran bindings cannot. - The bindings comparison reads optionality off the annotation, so a member absent on one side is skipped only where the class says it may be, and every other member is still compared. Also fixes a copy-paste in two benchmark setups: `dual_normal_vert_y` was wired to EDGE_NORMAL_VERTEX_V rather than EDGE_TANGENT_VERTEX_V. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF

CellParams moves to the attribute-docstring style EdgeParams uses, gains a class docstring, and cites the ICON source of `mean_cell_area`. The convention sentence is stated once per class, so the per-field note on `mean_cell_area` goes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF

ICON allocates `edge_vert_length(nproma, nblks_e, 2)` (mo_alloc_patches.f90 l.1026) and the grid file carries `edge_vert_distance` as (2, num_edges), so the savepoint holds two values per edge, not four. `E2C2VDim` is the 4-wide dimension. The label had no effect on the values, which is why `test_edge_vertex_distance` passes either way, but it contradicted the metadata of the attribute the field is registered under: `geometry_attributes.EDGE_VERTEX_DISTANCE` declares `(EdgeDim, E2VDim)`, and `common/tests/common/fixtures.py` feeds this accessor straight into `GridGeometry` under that name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF

havogt · 2026-09-09T07:28:06Z

+    defined in ICON in mo_model_domain.f90:t_grid_edges%dual_edge_length
+    """
+
+    edge_cell_distances: gtx.Field[[dims.EdgeDim, dims.E2CDim], float] | None = None


this is added in preparation for tmx

what is tmx?

the turbulence scheme #1359

havogt · 2026-09-09T07:29:07Z

    def edge_vert_length(self):
        """length of edge midpoint to vertex"""
-        return self._get_field("edge_vert_length", dims.EdgeDim, dims.E2C2VDim)
+        return self._get_field("edge_vert_length", dims.EdgeDim, dims.E2VDim)


drive-by fix unrelated to this PR

Good catch!

It seems that no one caught this because it is only used in cell_aw_verts, and only first two indices are used.

havogt · 2026-09-09T07:29:23Z

cscs-ci run default

havogt · 2026-09-09T07:29:36Z

cscs-ci run default

nfarabullini · 2026-09-09T07:57:16Z

+    defined in ICON in mo_model_domain.f90:t_grid_edges%dual_edge_length
+    """
+
+    edge_cell_distances: gtx.Field[[dims.EdgeDim, dims.E2CDim], float] | None = None


what is tmx?

- bindings utils: keep only what the code does not say. For `_optional_fields` that is why 'get_type_hints' is not used; the rest of both comments was restating the lines below them. - states: drop the optional-member sentence from both class docstrings, and give `edge_cell_distances` the blank line before its ICON reference that every other member has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF

github-actions · 2026-09-09T09:30:06Z

When developing, you can test your changes on CSCS CI before merge with the default pipeline: cscs-ci run default. This will run a default subset of tests.

You can pass options to override pipeline variables, for example:

cscs-ci run default;BACKENDS=gtfn_cpu;LEVELS=unit
cscs-ci run default;MODEL_SUBPACKAGES=common:driver;SESSIONS=model

Avoid running the pipeline for all tests when you are developing.

Available options are:

SESSIONS: model, model_mpi, or tools (correspond to nox sessions)
MODEL_SUBSETS: datatest, basic, or stencils (correspond to nox session selections)
MODEL_SUBPACKAGES: subpackages for non-MPI tests (last component, e.g. diffusion, driver)
MODEL_MPI_SUBPACKAGES: subpackages for MPI tests (as above)
BACKENDS: backends
GRIDS: grids for stencil tests (simple, icon_regional, or icon_global)
LEVELS: testing level for non-stencil tests (unit or integration)

For each option, all can be used as a shorthand for all possible values of that variable, e.g. LEVELS=all.

Multiple values can be given to each option with : used as the separator (; separates options and , separates pipelines).

See scripts/python/generate_ci_pipeline.py and noxfile.py for available values for each option.

The all pipeline can be run with cscs-ci run all. This will run all icon4py tests in CSCS CI which can be expensive. This pipeline runs on a schedule on main, and can be run when extensive validation is needed (e.g. before releases).

Merging

Once your PR is approved and ready for merging, add it to the merge queue. The merge CSCS CI pipeline will run automatically on the merge-queue branch and must pass before the PR is merged. A dummy merge check will be triggered on the PR itself since it's required to add a PR to the merge queue.

Optional Tests

To run benchmarks you can use:

cscs-ci run benchmark-bencher

For more detailed information please look at CI in the EXCLAIM universe.

OngChia

LGTM

OngChia · 2026-09-09T09:35:46Z

    def edge_vert_length(self):
        """length of edge midpoint to vertex"""
-        return self._get_field("edge_vert_length", dims.EdgeDim, dims.E2C2VDim)
+        return self._get_field("edge_vert_length", dims.EdgeDim, dims.E2VDim)


Good catch!

OngChia · 2026-09-09T09:39:55Z

    def edge_vert_length(self):
        """length of edge midpoint to vertex"""
-        return self._get_field("edge_vert_length", dims.EdgeDim, dims.E2C2VDim)
+        return self._get_field("edge_vert_length", dims.EdgeDim, dims.E2VDim)


It seems that no one caught this because it is only used in cell_aw_verts, and only first two indices are used.

`grid_states.EdgeParams` carries it as `edge_cell_distances` since #1462, so the granule's metric state no longer has to. `from_sources` needs only the metrics source now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF

Brings in #1462, which makes CellParams.area required, ahead of binding it in the VelocityAdvection constructor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017s66WxborSWhPes8eHxiSF

Cell areas are static geometry. Bind them with the other constant arguments that setup_program binds, next to area_edge, instead of passing them to both run methods on every call. The two asserts that existed only to narrow the Optional type of those arguments go as well: CellParams.area has been required since #1462, which the previous commit merged in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017s66WxborSWhPes8eHxiSF

havogt requested a review from OngChia September 8, 2026 18:59

havogt marked this pull request as ready for review September 8, 2026 18:59

havogt mentioned this pull request Sep 8, 2026

Add the tmx package, its configuration and static states #1457

Merged

havogt commented Sep 9, 2026

View reviewed changes

Comment thread model/common/src/icon4py/model/common/grid/states.py Outdated

havogt commented Sep 9, 2026

View reviewed changes

Comment thread model/common/src/icon4py/model/common/grid/states.py Outdated

havogt and others added 2 commits September 9, 2026 09:26

havogt commented Sep 9, 2026

View reviewed changes

Comment thread model/common/src/icon4py/model/common/grid/states.py Outdated

Apply suggestion from @havogt

96c018a

havogt commented Sep 9, 2026

View reviewed changes

Merge branch 'main' into edgeparams-dataclass

58e5c32

nfarabullini reviewed Sep 9, 2026

View reviewed changes

havogt requested a review from nfarabullini September 9, 2026 09:33

nfarabullini approved these changes Sep 9, 2026

View reviewed changes

havogt added this pull request to the merge queue Sep 9, 2026

OngChia approved these changes Sep 9, 2026

View reviewed changes

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 9, 2026

havogt merged commit 0d43086 into main Sep 9, 2026
43 checks passed

havogt deleted the edgeparams-dataclass branch September 9, 2026 16:15

Conversation

havogt commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes

Why now

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

havogt commented Sep 9, 2026

Uh oh!

havogt commented Sep 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 9, 2026

Uh oh!

OngChia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

havogt commented Sep 8, 2026 •

edited

Loading