NonHydrostaticConfig -> dataclass#1345
Conversation
|
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
To run tests and benchmarks with the DaCe backend you can use:
To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
There was a problem hiding this comment.
Pull request overview
This PR refactors NonHydrostaticConfig in the dycore to a declarative @dataclass configuration style (per ADR 0002), backed by common config option metadata for ICON namelist mapping, and adds supporting typing/mypy adjustments across the dycore.
Changes:
- Convert
NonHydrostaticConfigto a@dataclassusingtyping.Annotated[..., ConfigOption(...)]and delegate ICON-dict construction toconstruct_config_from_icon. - Extend common ICON option parsing with an optional per-option
converterhook. - Tighten/adjust type hints across dycore modules and expand mypy coverage to include dycore (with targeted ignores for known-problematic modules).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Enables mypy on dycore sources and adds dycore-specific ignore patterns. |
| model/common/src/icon4py/model/common/config/options.py | Adds IconOption.converter and applies it during ICON-dict-to-config construction. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/velocity_advection.py | Adds/adjusts return annotations and local mypy suppressions for array backend attributes. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/solve_nonhydro.py | Refactors NonHydrostaticConfig to a declarative dataclass and routes parsing through common config helpers; adds a few runtime guards. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/solve_nonhydro_stencils.py | Adds -> None return typing for a program helper. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/dycore_utils.py | Adds -> None return typing for GT4Py programs. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/dycore_states.py | Minor typing tweaks / ignores around scalar-like array typing and __post_init__ return type. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/stencils/vertically_implicit_dycore_solver.py | Adds explicit -> None return typing to program entrypoint. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/stencils/solve_tridiagonal_matrix_for_w_forward_sweep.py | Adds typing/ignore annotations around scan operator init/return types. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/stencils/solve_tridiagonal_matrix_for_w_back_substitution.py | Adds a mypy ignore for GT4Py scan operator return typing. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/stencils/compute_horizontal_velocity_quantities.py | Adds explicit -> None return typing to program entrypoint. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/stencils/compute_edge_diagnostics_for_dycore_and_update_vn.py | Adds explicit -> None return typing to program entrypoint. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/stencils/compute_cell_diagnostics_for_dycore.py | Adds explicit -> None return typing to program entrypoint. |
| model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/stencils/compute_advection_in_vertical_momentum_equation.py | Adds explicit -> None return typing to program entrypoint. |
| model/atmosphere/diffusion/src/icon4py/model/atmosphere/diffusion/diffusion.py | Aligns/shortens the nudging config description string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| converter = opt.icon_equivalent.converter or annotations[name] | ||
| yield name, converter(de_listified) |
| offset_provider=self._grid.connectivities, | ||
| ) | ||
|
|
||
| assert self._cell_params.mean_cell_area is not None |
There was a problem hiding this comment.
I think you can ignore copilot for these asserts.
Better long term would be to investigate if the members of CellParams etc. can be made non-nullable, but given that they are nullable I find the asserts ok.
| and not (at_initial_timestep and at_first_substep) | ||
| ) | ||
|
|
||
| assert self._cell_params.area is not None |
| assert self._cell_params.area is not None | ||
| assert self._cell_params.mean_cell_area is not None |
| init=( # type: ignore[call-overload] # GT4Py misses type hint for tuples here | ||
| vpfloat("0.0"), | ||
| 0.0, | ||
| ), # boundary condition for upper tridiagonal element and w at model top |
| c: vpfloat, | ||
| d: wpfloat, | ||
| ): | ||
| ) -> tuple[wpfloat, wpfloat]: |
jcanton
left a comment
jcanton
left a comment
There was a problem hiding this comment.
LGTM
just two minor comments / questions more for myself than anything
|
|
||
| # `max_vertical_cfl` stored as 0-d array of type ta.wpfloat (to be able to avoid cupy synchronization) | ||
| max_vertical_cfl: data_alloc.ScalarLikeArray[ta.wpfloat] | ||
| max_vertical_cfl: data_alloc.ScalarLikeArray[ta.wpfloat] # type: ignore[type-var] # TODO(ricoh): find out what this is about |
There was a problem hiding this comment.
you mean the variable itself or the reason behind the type ignore?
| """ | ||
|
|
||
| horizontal_pressure_gradient: fa.EdgeKField[ta.vpfloat] | ||
| horizontal_pressure_gradient: fa.EdgeKField[float] |
There was a problem hiding this comment.
ignorance / question for myself: why these changed type?
| ) | ||
|
|
||
|
|
||
| @dataclasses.dataclass |
msimberg
left a comment
msimberg
left a comment
There was a problem hiding this comment.
No blocking comments, thanks!
| "Maximal value of the nudging coefficients used cell row bordering the boundary interpolation zone," | ||
| "from there nudging coefficients decay exponentially with `nudge_efold_width` in units of cell rows." | ||
| ), | ||
| description="Maximum relaxation coefficient for lateral boundary nudging", |
There was a problem hiding this comment.
I like the shortening, but maybe a scientist can comment whether the extra verbosity in the old version was useful or not?
There was a problem hiding this comment.
we thought we'd use the short / first line commetn in icon-nwp/doc/www/documentation/buildrun/buildrun_namelist_overview.md
then it's consistent everywhere and if more verbose info is needed people can go look up the docs and we avoid duplicating here info that may become outdated
There was a problem hiding this comment.
should be found here too:
https://docs.icon-model.org/documentation/buildrun/buildrun_namelist_overview.html
do you think we should mention this and / or add the link for reference?
| vertical_start_index_model_top: gtx.int32, | ||
| vertical_end_index_model_surface: gtx.int32, | ||
| ): | ||
| ) -> None: |
There was a problem hiding this comment.
Just for me to understand: all these new type hints and type ignores come from you enabling mypy for these files and are not strictly required for the config changes?
Keep the changes (and thank you for fixing them), I'm just asking to understand that there isn't another reason (actually related to the config) for changing these now.
| offset_provider=self._grid.connectivities, | ||
| ) | ||
|
|
||
| assert self._cell_params.mean_cell_area is not None |
There was a problem hiding this comment.
I think you can ignore copilot for these asserts.
Better long term would be to investigate if the members of CellParams etc. can be made non-nullable, but given that they are nullable I find the asserts ok.
Turn NonHydrostaticConfig into a dataclass in declarative style (as described in ADR 0002).