Skip to content

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

Draft
Copilot wants to merge 2 commits intomainfrom
copilot/clean-up-global-grid-parameters
Draft

[WIP] Clean up global grid parameters and grid shape#1183
Copilot wants to merge 2 commits intomainfrom
copilot/clean-up-global-grid-parameters

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 30, 2026

  • Clean up GlobalGridParams dataclass (model/common/src/icon4py/model/common/grid/icon.py):
    • Make grid_shape required (non-Optional)
    • Remove global_num_cells field (theoretical full-sphere count, not used in production)
    • Remove num_edges and num_vertices fields (only used in test definitions, grid size info belongs in GridConfig)
    • Remove characteristic_length field (dead field, never populated - computed in geometry.py)
    • Remove limited_area field (redundant with GridConfig.limited_area)
    • Remove compute_mean_cell_area_for_sphere function (dead code, never called)
    • Clean up __post_init__ to reflect removed fields
    • Update geometry_type and subdivision properties (no Optional return)
  • Move fields to GridDescription (model/testing/src/icon4py/model/testing/definitions.py):
    • Add num_edges, num_vertices, limited_area fields to GridDescription
    • Update all grid description instances
  • Update grid_manager.py: Remove global_num_cells from params construction
  • Update initialization_utils.py (driver): Update GlobalGridParams construction
  • Update py2fgen common.py: Update GlobalGridParams() construction
  • Update test files:
    • test_icon.py: Remove global_num_cells, mean_cell_area from test parameters
    • test_grid_manager.py: Update test that checks global_num_cells, update params.num_edges/params.num_vertices references
    • test_gridfile.py: Update to use grid_descriptor fields
    • test_grid_refinement.py: Update params.limited_area references
    • MPI test files: Update params.limited_area references
  • Run tests to verify changes
  • Code review and security scan

@jenkins-apn
Copy link
Copy Markdown

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

1 similar comment
@jenkins-apn
Copy link
Copy Markdown

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

Copilot AI linked an issue Mar 30, 2026 that may be closed by this pull request
Agent-Logs-Url: https://github.com/C2SM/icon4py/sessions/148d4123-da81-4914-a9e3-16465bc49c48

Co-authored-by: jcanton <5622559+jcanton@users.noreply.github.com>
Comment on lines 74 to -84
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
Copy link
Copy Markdown
Contributor

@msimberg msimberg Apr 14, 2026

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

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

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

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

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

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!

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.

Clean up global grid parameters and grid shape

5 participants