Skip to content

[WIP] Clean up global grid parameters and grid shape #1183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[WIP] Clean up global grid parameters and grid shape #1183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34de548
Initial plan
Copilot Mar 30, 2026
9333935
Changes before error encountered
Copilot Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 1 addition & 32 deletions model/common/src/icon4py/model/common/grid/icon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
# SPDX-License-Identifier: BSD-3-Clause
import dataclasses
import logging
import math
from collections.abc import Callable
from typing import Final, TypeVar

Expand Down Expand Up @@ -73,15 +72,10 @@ def __init__(
@dataclasses.dataclass(kw_only=True, frozen=True)
class GlobalGridParams:
grid_shape: Final[GridShape | None] = None
radius: float = constants.EARTH_RADIUS
radius: float | None = constants.EARTH_RADIUS
domain_length: float | None = None
domain_height: float | None = None
global_num_cells: int | None = None
num_cells: int | None = None
num_edges: int | None = None
num_vertices: int | None = None
characteristic_length: float | None = None
limited_area: bool = False
Comment on lines 74 to -84
Copy link
Copy Markdown
Contributor

@msimberg msimberg Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I had more in mind for this is something like:

  • torus_or_icosahedron_params: IcosahedronParams | TorusParams (with a better variable name)
  • num_cells: int
  • num_edges: int
  • num_vertices: int
  • limited_area: bool

More concretely:

  • no None parameters, we should almost always be able to figure out what the parameters are
  • following from the previous, torus/icosahedron parameters should be variant-like, not sometimes set, sometimes not set
  • either all of num cells/edges/vertices are stored here, or none are
  • limited area: not sure if this belongs here or not
  • charecteristic length: probably belongs next to the mean edge lengths etc., it's directly derived from them

@jcanton @nfarabullini @OngChia do you have opinions on what this should look like?

Copy link
Copy Markdown
Contributor

@nfarabullini nfarabullini Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are my 2 cents:

  • why did you add None to radius if it anyway has a default value? The Nonetype is only specified because the default value is None
  • how about meshParams as an alternative to torus_or_icosahedron_params?
  • where would you place limited_area instead?
  • I'm ok if you move charecteristic_length

Copy link
Copy Markdown
Contributor

@msimberg msimberg Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Because it doesn't make sense for torus to have a radius. It was a poor solution.
  • Mesh params sounds like a more promising name, I'm also considering keeping something related to "grid" and "shape"... I think we've tried to avoid mixing "grid" and "mesh" since they are mostly the same as far as I know, and so far we prefer "grid"
  • It's also in GridConfig. I don't know yet if we can keep only one of these.
  • 👍 for characteristic length

Copy link
Copy Markdown
Contributor

@msimberg msimberg Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more: num_cells, num_edges, num_vertices also have a helper class: HorizontalGridSize. We should probably reuse that here, or remove it from here if HorizontalGridSize anyway lives on its own.

Copy link
Copy Markdown
Contributor

@nfarabullini nfarabullini Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok then how about CustomGridParams because simply GridParams might be too generic. Thank you for answer the other questions. As far as I'm concerned, you can go ahead with the changes

Copy link
Copy Markdown
Contributor

@msimberg msimberg Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agree on GridParams being quite generic. Not sure Custom makes it less generic, but I'll have a think about it. Thanks for the suggestions!


def __post_init__(self) -> None:
if self.geometry_type is not None:
Expand All @@ -94,16 +88,6 @@ def __post_init__(self) -> None:
case base.GeometryType.TORUS:
object.__setattr__(self, "radius", None)

if self.global_num_cells is None and self.geometry_type is base.GeometryType.ICOSAHEDRON:
object.__setattr__(
self,
"global_num_cells",
compute_icosahedron_num_cells(self.grid_shape.subdivision),
)

if self.num_cells is None and self.global_num_cells is not None:
object.__setattr__(self, "num_cells", self.global_num_cells)

@property
def geometry_type(self) -> base.GeometryType | None:
return self.grid_shape.geometry_type if self.grid_shape else None
Expand All @@ -117,21 +101,6 @@ def compute_icosahedron_num_cells(subdivision: GridSubdivision) -> int:
return 20 * subdivision.root**2 * 4**subdivision.level


def compute_mean_cell_area_for_sphere(radius, num_cells) -> float:
"""
Compute the mean cell area.

Computes the mean cell area by dividing the sphere by the number of cells in the
global grid.

Args:
radius: average earth radius, might be rescaled by a scaling parameter
num_cells: number of cells on the global grid
Returns: mean area of one cell [m^2]
"""
return 4.0 * math.pi * radius**2.0 / num_cells


@dataclasses.dataclass(frozen=True)
class IconGrid(base.Grid):
global_properties: GlobalGridParams = dataclasses.field(default=None, kw_only=True)
Expand Down
29 changes: 17 additions & 12 deletions model/common/tests/common/grid/unit_tests/test_grid_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,6 @@
from .. import utils


MCH_CH_RO4B09_GLOBAL_NUM_CELLS = 83886080


# TODO @magdalena add test cases for hexagon vertices v2e2v
# v2e2v: grid,???

Expand Down Expand Up @@ -322,9 +319,9 @@ def test_grid_manager_grid_size(
backend: gtx_typing.Backend, grid_descriptor: definitions.GridDescription
) -> None:
grid = utils.run_grid_manager(grid_descriptor, keep_skip_values=True, backend=backend).grid
assert grid_descriptor.params.num_cells == grid.size[dims.CellDim]
assert grid_descriptor.params.num_edges == grid.size[dims.EdgeDim]
assert grid_descriptor.params.num_vertices == grid.size[dims.VertexDim]
assert grid_descriptor.num_cells == grid.size[dims.CellDim]
assert grid_descriptor.num_edges == grid.size[dims.EdgeDim]
assert grid_descriptor.num_vertices == grid.size[dims.VertexDim]


def assert_up_to_order(
Expand Down Expand Up @@ -368,20 +365,28 @@ def test_gt4py_transform_offset_by_1_where_valid(size: int) -> None:


@pytest.mark.parametrize(
"grid_descriptor, global_num_cells",
"grid_descriptor, expected_subdivision",
[
(definitions.Grids.R02B04_GLOBAL, definitions.Grids.R02B04_GLOBAL.params.num_cells),
(definitions.Grids.MCH_CH_R04B09_DSL, MCH_CH_RO4B09_GLOBAL_NUM_CELLS),
(
definitions.Grids.R02B04_GLOBAL,
icon.GridSubdivision(root=2, level=4),
),
(
definitions.Grids.MCH_CH_R04B09_DSL,
icon.GridSubdivision(root=4, level=9),
),
],
)
def test_grid_manager_grid_level_and_root(
grid_descriptor: definitions.GridDescription, global_num_cells: int, backend: gtx_typing.Backend
grid_descriptor: definitions.GridDescription,
expected_subdivision: icon.GridSubdivision,
backend: gtx_typing.Backend,
) -> None:
assert (
global_num_cells
expected_subdivision
== utils.run_grid_manager(
grid_descriptor, keep_skip_values=True, backend=backend
).grid.global_properties.global_num_cells
).grid.global_properties.subdivision
)


Expand Down
78 changes: 10 additions & 68 deletions model/common/tests/common/grid/unit_tests/test_icon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from __future__ import annotations

import functools
import math
import re
from typing import TYPE_CHECKING

Expand Down Expand Up @@ -230,65 +229,25 @@ def test_when_replace_skip_values_then_only_pentagon_points_remain(
assert connectivity.skip_value is None


def _sphere_area(radius: float) -> float:
return 4.0 * math.pi * radius**2.0


@pytest.mark.parametrize(
"geometry_type,grid_root,grid_level,global_num_cells,num_cells,mean_cell_area,expected_global_num_cells,expected_num_cells,expected_mean_cell_area",
"geometry_type,grid_root,grid_level,num_cells,expected_num_cells",
[
(
base.GeometryType.ICOSAHEDRON,
1,
0,
None,
None,
None,
20,
20,
_sphere_area(constants.EARTH_RADIUS) / 20,
),
(
base.GeometryType.ICOSAHEDRON,
1,
1,
None,
None,
None,
20 * 4,
20 * 4,
_sphere_area(constants.EARTH_RADIUS) / (20 * 4),
),
(
base.GeometryType.ICOSAHEDRON,
1,
2,
None,
None,
None,
20 * 16,
20 * 16,
_sphere_area(constants.EARTH_RADIUS) / (20 * 16),
),
(base.GeometryType.ICOSAHEDRON, 2, 4, None, None, None, 20480, 20480, 24907282236.708576),
(base.GeometryType.ICOSAHEDRON, 4, 9, 765, None, None, 765, 765, 666798876088.6165),
(base.GeometryType.ICOSAHEDRON, 2, 4, None, 42, 123.456, 20480, 42, 123.456),
(base.GeometryType.ICOSAHEDRON, 4, 9, None, None, 123.456, 83886080, 83886080, 123.456),
(base.GeometryType.ICOSAHEDRON, 4, 9, None, 42, None, 83886080, 42, 6080879.45232143),
(base.GeometryType.TORUS, 2, 0, None, 42, 123.456, None, 42, 123.456),
(base.GeometryType.TORUS, None, None, None, 42, None, None, 42, None),
(base.GeometryType.ICOSAHEDRON, 1, 0, None, None),
(base.GeometryType.ICOSAHEDRON, 1, 1, None, None),
(base.GeometryType.ICOSAHEDRON, 1, 2, None, None),
(base.GeometryType.ICOSAHEDRON, 2, 4, None, None),
(base.GeometryType.ICOSAHEDRON, 2, 4, 42, 42),
(base.GeometryType.ICOSAHEDRON, 4, 9, 42, 42),
(base.GeometryType.TORUS, 2, 0, 42, 42),
(base.GeometryType.TORUS, None, None, 42, 42),
],
)
def test_global_grid_params(
geometry_type: base.GeometryType,
grid_root: int | None,
grid_level: int | None,
global_num_cells: int | None,
num_cells: int | None,
mean_cell_area: float | None,
expected_global_num_cells: int | None,
expected_num_cells: int | None,
expected_mean_cell_area: float | None,
) -> None:
if grid_root is None:
assert grid_level is None
Expand All @@ -303,7 +262,6 @@ def test_global_grid_params(
),
domain_length=42.0,
domain_height=100.5,
global_num_cells=global_num_cells,
num_cells=num_cells,
)
assert geometry_type == params.geometry_type
Expand All @@ -322,7 +280,6 @@ def test_global_grid_params(
assert pytest.approx(params.radius) == constants.EARTH_RADIUS
assert params.domain_length is None
assert params.domain_height is None
assert params.global_num_cells == expected_global_num_cells
assert params.num_cells == expected_num_cells


Expand All @@ -348,7 +305,7 @@ def test_grid_shape_fail(geometry_type: base.GeometryType, grid_root: int, grid_

@pytest.mark.datatest
@pytest.mark.parametrize(
"grid_descriptor, geometry_type, subdivision, radius, domain_length, domain_height, global_num_cells, num_cells, characteristic_length",
"grid_descriptor, geometry_type, subdivision, radius, domain_length, domain_height, num_cells",
[
(
definitions.Grids.R02B04_GLOBAL,
Expand All @@ -358,8 +315,6 @@ def test_grid_shape_fail(geometry_type: base.GeometryType, grid_root: int, grid_
None,
None,
20480,
20480,
157817.27689721118,
),
(
definitions.Grids.MCH_OPR_R04B07_DOMAIN01,
Expand All @@ -368,9 +323,7 @@ def test_grid_shape_fail(geometry_type: base.GeometryType, grid_root: int, grid_
constants.EARTH_RADIUS,
None,
None,
5242880,
10700,
9379.079256436624,
),
(
definitions.Grids.MCH_OPR_R19B08_DOMAIN01,
Expand All @@ -379,9 +332,7 @@ def test_grid_shape_fail(geometry_type: base.GeometryType, grid_root: int, grid_
constants.EARTH_RADIUS,
None,
None,
473169920,
44528,
1014.8736406119558,
),
(
definitions.Grids.MCH_CH_R04B09_DSL,
Expand All @@ -390,9 +341,7 @@ def test_grid_shape_fail(geometry_type: base.GeometryType, grid_root: int, grid_
constants.EARTH_RADIUS,
None,
None,
83886080,
20896,
2501.209495453326,
),
(
definitions.Grids.TORUS_100X116_1000M,
Expand All @@ -401,9 +350,7 @@ def test_grid_shape_fail(geometry_type: base.GeometryType, grid_root: int, grid_
None,
100000.0,
100458.94683899487,
None,
23200,
658.0370064762462,
),
(
definitions.Grids.TORUS_50000x5000,
Expand All @@ -412,9 +359,7 @@ def test_grid_shape_fail(geometry_type: base.GeometryType, grid_root: int, grid_
None,
50000.0,
5248.638810814779,
None,
1056,
498.51288369412595,
),
],
)
Expand All @@ -426,9 +371,7 @@ def test_global_grid_params_from_grid_manager(
radius: float,
domain_length: float,
domain_height: float,
global_num_cells: int,
num_cells: int,
characteristic_length: float,
) -> None:
grid = utils.run_grid_manager(grid_descriptor, keep_skip_values=True, backend=backend).grid
params = grid.global_properties
Expand All @@ -444,5 +387,4 @@ def test_global_grid_params_from_grid_manager(
assert pytest.approx(params.radius) == radius
assert pytest.approx(params.domain_length) == domain_length
assert pytest.approx(params.domain_height) == domain_height
assert params.global_num_cells == global_num_cells
assert params.num_cells == num_cells
Loading