Add the tmx package, its configuration and static states - #1457
Conversation
…ates First of a series carving the tmx port (#1359, author @jcanton) into independently reviewable pieces. This one stands the package up and brings the parts that need no operators, so the later PRs are one subcomponent each. Package: workspace member, tach module (depends on common only), nox and CI matrix entries. The uv.lock is hand-edited rather than regenerated, because a newer uv reformats the whole file; `uv lock --check` accepts it. TmxConfig lives in its own module: the granule imports every stencil at module level, so reading a config would otherwise pull them all in. Verified that importing it loads no stencil module. Only the static states are here. The diagnostic, tendency, new and input states are the subject of an ongoing design discussion and arrive with the subcomponents that use them. Half-level fields are typed on KHalfDim, dropping the port's workaround that re-tagged them back to KDim. That needed three additions in common: compute_reciprocal_on_cell_khalf, a program wrapping the existing _cell_2_edge_interpolation_on_half_levels, and the field operators behind them. test_static_fields validates the result against the serialized reference and needs a compiled backend; it is xfail on embedded. Also from the port, each with a consumer here: the metrics factory fields tmx needs, positional derived-type namelist support in the common config options, and three savepoints. Co-authored-by: Jacopo Canton <jacopo.canton@mpimet.mpg.de> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
Seven accessors described half-level quantities but read them as full-level fields: the three inverse half-level thicknesses, the geopotential above ground, the Smagorinsky mixing length, the vertical wind and the interface pressure. #1359 wrote them before #1429 made half levels their own dimension, and they came over unexamined. test_static_fields did not catch it because it compares values rather than dimensions. It surfaces as soon as one of these fields is passed to a program, since a program argument carries its grid. Found while building the diagnostics component on top of this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
Two review items from #1457. TmxConfig._validate existed only to be called by __post_init__, which is where validation belongs; it is now inline. The comment explaining the uses_concat_where marker sat above the marker and read as detached from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
Replaces the four numpy quadratic-extrapolation weight functions with gt4py, as asked on #1457. The three coefficients are the same expression of four interface heights, so one operator computes them and each level selects its own; because all three levels read the same four heights, the shift differs per level: from full level k, interface j is at KDim + (j - k) - 0.5. Verified bit-for-bit against the numpy formula on embedded and gtfn_cpu before wiring it in. The two edge variants need no code of their own. They are a cell-to-edge interpolation of the cell weights, so they reuse the operator already in common, and the halo exchange moves from inside the function to the factory entry that owns it -- the second review point on the same file. This needed one thing from the factory. A vertical sub-range in a provider's compute domain is ambiguous: either the field spans the column and only part of it is computed, as for rayleigh_w and coeff_dwdz, or the field exists on those levels alone, as here. ProgramFieldProvider now takes vertically_bounded_by_domain to say which, defaulting to the former, so these weights stay three-row fields carrying their absolute level indices. Co-authored-by: Jacopo Canton <jacopo.canton@mpimet.mpg.de> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
Everything here lives on the surface, one horizontal layer, so it is an ordinary CellDim stencil. The surface pressure is the bottom interface row of pressure_ifc, taken as a view of the live buffer rather than copied, so the provider still reads whatever the caller updated this step. sat_pres_water and specific_humidity gain field operators beside their numpy versions, and PhysicsConstants gains the four values they reference: gtfn needs constants used inside an operator to come from the wpfloat enum. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
The flux providers have no caller here -- only their own test -- which is what the review pointed out. They belong with the component that consumes them, so they move to their own PR: the providers, the surface-flux state and its savepoint, the test, and the two thermodynamic field operators with the four constants that nothing else uses. The state-allocation test case goes with the state; it had no other state left to exercise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
The metric and interpolation fields tmx uses are registered in the common factories, so their comparison against the savepoints belongs next to the providers. The nine interpolation fields were already covered there by test_interpolation_factory; the metric fields this branch adds had no test in test_metrics_factory at all. - test_metrics_factory: cover inv_ddqz_z_half, inv_ddqz_z_full_e, inv_ddqz_z_half_e, inv_ddqz_z_half_v, wgtfacq1_c, wgtfacq1_e, geopot_agl_ifc and height_above_ground. The first six are pinned to the AES physics experiment, whose 'tmx-init' savepoint holds the only serialized reference; height_above_ground is checked against 'z_mc - z_ifc[:, -1:]' taken from the savepoints, not against the function under test. - grid_utils: key the cached grid geometry on the number of levels too. The two aquaplanet experiments share the R02B04 grid file, so without it the first one to run fixes num_levels for the other. - tmx: replace the static-fields datatest with a unit test of the 'from_sources' wiring, which needs no serialized data. Its savepoint helpers move to the component PR that still uses them. - tmx: drop test_default_config_matches_fortran_defaults. It asserted the dataclass defaults against literals copied from that dataclass; test_tmx_namelist_config checks them against what ICON's vdiff_config_init echoes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
| self._do_exchange = do_exchange | ||
|
|
||
| def _field_extent(self, dim: gtx.Dimension, grid: GridProvider) -> tuple[int, int]: | ||
| """The extent to allocate for ``dim``. |
There was a problem hiding this comment.
I did not know this.
So, just to confirm, we will create CellKField fields such as the quadratic extrapolation coefficients wgtfacq_c or wgtfacq1_c with only three levels in the vertical coordinate.
| z_ifc(dims.KDim + 0.5), | ||
| z_ifc(dims.KDim + 1.5), | ||
| ) | ||
| _, _, w3 = _quadratic_extrapolation_weights( |
There was a problem hiding this comment.
This workaround of cyclic permutation of vertical levels to compute simple quadratic extrapolation weights seems not very ideal and hard to swallow. I guess this is because of gt4py limitations.
- tmx_states, metric_fields: the docstring wordings he suggested, taken as proposed. - compute_weight_factors: drop `_compute_z1_z2_z3`, left behind by the gt4py conversion and called from nowhere. - serialbox: drop the `ta_phy` accessor. Nothing reads it, and the name is a trap: in ICON `ta_phy` is a member of the tendency list (mo_aes_phy_memory.f90 l.544, [K/s]), but the field serialized under that name in 'tmx-entry' is bit-identical to `ta`. - the ISO 8601 duration parser moves to common/utils/time_utils.py, with its unit tests; the driver and the tmx fixtures each had a copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
…mbers (#1462) `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.
`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
The four interface heights are the same for all three coefficients; only their address relative to the level being written changes. Selecting them with concat_where evaluates the extrapolation once instead of three times with two of every three results discarded. Out-of-range shifts stay out of range only because concat_where restricts each branch to its own region, as before — the reads just moved from the outputs to the inputs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
Brings in #1463, which makes a ProgramFieldProvider's declared vertical range the field's extent for every provider. That supersedes this branch's opt-in `vertically_bounded_by_domain`: - states/factory.py: main's version; this branch's changes there were the flag and nothing else. - metrics_factory.py: this branch's version with #1463's rayleigh_w and coeff_dwdz domain fixes applied, the four `vertically_bounded_by_domain=True` dropped, and geopot_agl_ifc's `gtx.domain(...)` replaced by the bare dimensions, which NumpyDataProvider no longer accepts. Main's numpy wgtfacq_c/wgtfacq_e providers stay replaced by this branch's gt4py ones. Also fixes the "acurate" typo in the coeff_dwdz docstring that #1463 carried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
- test_tmx_states.py and `from_sources` go: the tests restated the mapping and checked common's metadata registry, and nothing calls `from_sources` yet. - `_DiffusedTracer` leaves config.py: it is runtime data, used by the scalar diffusion, not configuration. - `unnamed_index` gets its own tests in common's test_options.py; it was only exercised through TmxConfig. - The tmx-entry savepoint, `mix_len_sq`, `scaling_factor_louis`, the `tmx_dtime` fixture and the unused datatest fixture re-exports move to the diagnostics PR; the surface-flux options (read from nh_testcase_nml) move to #1458 with their only consumer. - `compute_geopot_agl_ifc` is named after the operation: `compute_geopotential_above_ground_on_half_levels`. - test_package.py goes; no peer package has one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
- tmx: add the README the package metadata declares; drop the unused logging in config.py, the dead `z_mc`/`z_ifc` state members, and the stencil fixture re-exports no test here uses. - common: the new inverse operators move next to `compute_inverse_on_edges` in math/utils.py as `compute_inverse_on_cell_khalf`/`_edge_k`, with the same zero guard, instead of a second "reciprocal" family in another module. - The edge/vertex inverse half-level fields leave with their only consumer, the diffusion: attributes, providers, `cell_2_edge_interpolation_on_half_levels`, their factory test, savepoint accessors and state members. - test_compute_wgtfacq_e_dsl goes: it ran cell_2_edge_interpolation only and duplicated test_factory_wgtfacq_e. The `relativetime_from_iso8601` re-export in common.utils had no user. - Renames: `_compute_quadratic_extrapolation_weights`, `compute_wgtfacq_c` (the `_dsl` suffix was the numpy version's), and one `compute_height_above_surface` helper, with the geopotential as GRAV times it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
The reference was the formula under test applied to the savepoint's z_mc and z_ifc, so only the provider wiring was checked. It now comes from ICON's serialized geopot_agl_ifc, averaged onto full levels and divided by GRAV, which is independent of our formula: taking the surface from the top row instead of the bottom fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF
|
When developing, you can test your changes on CSCS CI before merge with the You can pass options to override pipeline variables, for example:
Avoid running the pipeline for all tests when you are developing. Available options are:
For each option, Multiple values can be given to each option with See The Merging Once your PR is approved and ready for merging, add it to the merge queue. The Optional Tests To run benchmarks you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
yiluchen1066
left a comment
There was a problem hiding this comment.
Thanks for the PR! I did not find something need to be changed urgently. Mostly minor things. We can merge this one pretty soon!
| """ | ||
| Type of the vertical diffusion solver. | ||
|
|
||
| Note: Called ``solver_type`` in ``mo_turb_vdiff_config.f90``. |
There was a problem hiding this comment.
| Note: Called ``solver_type`` in ``mo_turb_vdiff_config.f90``. |
No need to keep this, also for the rest of the codes.
|
|
||
|
|
||
| @config_io.register_enum | ||
| class TurbulenceSolverType(int, enum.Enum): |
There was a problem hiding this comment.
shall we rename this to TmxSolverType?
| """ | ||
| Construct the configuration from the echoed ICON namelist. | ||
|
|
||
| ``aes_vdf_nml`` is a derived-type namelist (``t_vdiff_config``), which | ||
| ICON echoes as an anonymous positional array of the member values in | ||
| declaration order, so the options are located by ``unnamed_index`` | ||
| (pinned to mo_turb_vdiff_config.f90) instead of by name. Only the | ||
| first domain is read. The guards below make a change of the Fortran | ||
| type fail loudly instead of silently mis-assigning values. | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Construct the configuration from the echoed ICON namelist. | |
| ``aes_vdf_nml`` is a derived-type namelist (``t_vdiff_config``), which | |
| ICON echoes as an anonymous positional array of the member values in | |
| declaration order, so the options are located by ``unnamed_index`` | |
| (pinned to mo_turb_vdiff_config.f90) instead of by name. Only the | |
| first domain is read. The guards below make a change of the Fortran | |
| type fail loudly instead of silently mis-assigning values. | |
| """ | |
| """ | |
| Build the configuration from the echoed ICON namelist. | |
| ICON writes ```aes_vdf_config``` values in Fortran member order, without names. | |
| We read the first domain using each options's ```unnamed_index```. The checks below | |
| help detect changes to the expected Fortran layout | |
| """ |
let's simplify the doc string a bit.
| # number of members of the Fortran t_vdiff_config derived type | ||
| # (mo_turb_vdiff_config.f90); the echoed aes_vdf_nml namelist holds this | ||
| # many values per domain, in declaration order. Must be kept in sync with | ||
| # the 'unnamed_index' positions of the options above. | ||
| num_members = 42 | ||
| # position of 'use_tmx' in t_vdiff_config, used as an order canary | ||
| use_tmx_index = 22 |
There was a problem hiding this comment.
| # number of members of the Fortran t_vdiff_config derived type | |
| # (mo_turb_vdiff_config.f90); the echoed aes_vdf_nml namelist holds this | |
| # many values per domain, in declaration order. Must be kept in sync with | |
| # the 'unnamed_index' positions of the options above. | |
| num_members = 42 | |
| # position of 'use_tmx' in t_vdiff_config, used as an order canary | |
| use_tmx_index = 22 | |
| # Layout of t_vdiff_config in mo_turb_vdiff_config.f90 | |
| # Keep these values and the options' unnamed_index positions in sync | |
| num_members = 42 | |
| use_tmx_index = 22 |
| return config_cls( | ||
| **dict(iter_pairs_from_icon(config_cls, icon_config)), | ||
| **overrides, | ||
| ) |
There was a problem hiding this comment.
I was testing GPT-6 Astra, and it caught this here:
| values = dict(iter_pairs_from_icon(config_cls, icon_config)) | |
| values.update(overrides) | |
| return config_cls(**values) |
Both dictionaries can contain km_const which caused the error
|
|
||
| @pytest.mark.datatest | ||
| def test_compute_wgtfacq_c_dsl( | ||
| def test_compute_wgtfacq_c( |
There was a problem hiding this comment.
| def test_compute_wgtfacq_c( | |
| @pytest.mark.datatest | |
| @pytest.mark.uses_concat_where | |
| def test_compute_wgtfacq_c( |
| def wgtfacq1_c(self): | ||
| # Top-extrapolation coefficients: unlike `wgtfacq_c` (bottom extrapolation, stored | ||
| # surface-first, i.e. reversed w.r.t. increasing k, hence flipped in its accessor), | ||
| # `wgtfacq1_c(jc,k,jb)` with k=1..3 multiplies the full level k counted from the model | ||
| # top (mo_vertical_grid.f90 ll. 955-968), which already matches icon4py's top-down KDim | ||
| # orientation. No flip needed. | ||
| return self._get_field("wgtfacq1_c", dims.CellDim, dims.KDim) | ||
|
|
||
| def wgtfacq1_e(self): | ||
| # No flip, see `wgtfacq1_c`. Usage in mo_vdf_atmo.f90 (interpolate_normal_velocity_ | ||
| # edge_interface, ll. 1247-1250): vn_ie(je,1,jb) = sum_k wgtfacq1_e(je,k,jb)*vn(je,k,jb). | ||
| return self._get_field("wgtfacq1_e", dims.EdgeDim, dims.KDim) |
There was a problem hiding this comment.
| def wgtfacq1_c(self): | |
| # Top-extrapolation coefficients: unlike `wgtfacq_c` (bottom extrapolation, stored | |
| # surface-first, i.e. reversed w.r.t. increasing k, hence flipped in its accessor), | |
| # `wgtfacq1_c(jc,k,jb)` with k=1..3 multiplies the full level k counted from the model | |
| # top (mo_vertical_grid.f90 ll. 955-968), which already matches icon4py's top-down KDim | |
| # orientation. No flip needed. | |
| return self._get_field("wgtfacq1_c", dims.CellDim, dims.KDim) | |
| def wgtfacq1_e(self): | |
| # No flip, see `wgtfacq1_c`. Usage in mo_vdf_atmo.f90 (interpolate_normal_velocity_ | |
| # edge_interface, ll. 1247-1250): vn_ie(je,1,jb) = sum_k wgtfacq1_e(je,k,jb)*vn(je,k,jb). | |
| return self._get_field("wgtfacq1_e", dims.EdgeDim, dims.KDim) | |
| def wgtfacq1_c(self): | |
| return self._get_field("wgtfacq1_c", dims.CellDim, dims.KDim) | |
| def wgtfacq1_e(self): | |
| return self._get_field("wgtfacq1_e", dims.EdgeDim, dims.KDim) |
let's remove the docs here
| ) | ||
|
|
||
|
|
||
| def load_fortran_dict( |
There was a problem hiding this comment.
shall we move this to model/testing/datatest_utils since it uses the helper function get_path_for_experiment and potentially other components would also need this helper
The tmx port (#1359, @jcanton) is ~16k lines across 155 files. This is the first of a series carving it into independently reviewable pieces: the package plus everything that needs no operators. Each later PR adds one subcomponent with its own datatest (diagnostics, scalar and momentum diffusion, energy update). Prescribed surface fluxes, including their configuration options, are in #1458, stacked on this one.
What is here
pyproject.toml, README, workspace member,tachmodule (depends oncommononly), nox session, CI matrix entry.TmxConfigand its enums, intmx/config.py, so that reading the configuration does not import every stencil package the granule does.TmxMetricStateandTmxInterpolationState.wgtfacq1_c/e, the inverse layer thicknesses (cells on half levels, edges on full levels), geopotential and height above ground — each with a savepoint test intest_metrics_factory.py.wgtfacq_c/eand the newwgtfacq1_c/ereplace the numpy implementations.tmx_init) and the test-data version bump.unnamed_indexin the common config options (positional derived-type namelists), with its tests in common.common/utils/time_utils.py; the driver had its own copy.get_grid_geometrykeys its cache on the number of levels as well, since two experiments share the R02B04 grid file.Not here
The states that flow between subcomponents (
TmxInputState,TmxDiagnosticState, …) and everything with its first consumer in a later PR arrive with that PR.Differences from #1359
KHalfDim(Port tmx turbulence #1359 predates Use gt4py staggered dimensions for KHalfDim #1429); the port's re-tagging helper is gone.edge_cell_lengthcomes fromEdgeParams(refactor(common): EdgeParams as a dataclass with explicit optional members #1462) rather than the metric state.Testing
gtfn_cpu,-n0. The embedded backend is not evidence here: several of these tests are xfail on it.test_metrics_factory.py🤖 Generated with Claude Code
https://claude.ai/code/session_01QVSWPQauF9gNjADtuSTjxF