2026-04-28 GLM 5.1 correctness review

This was done by GLM-5.1. Requested by @msimberg. Done on commit ab2c3ecba9084a39ec2146c1ca07b4d3b76657a2. # Correctness Review Focus: bugs, typos, logic errors, incorrect computations, confusion. Not: architecture, style, performance (unless related to correctness). --- ## HIGH Severity ### H1. `mpi_size` assigned `my_rank()` instead of communicator size **File:** `model/common/src/icon4py/model/common/orchestration/decorator.py:206` **Status:** VERIFIED VALID (2025-05-04: removed in https://github.com/C2SM/icon4py/pull/1227) Line 206 reads `mpi_size = exchange_obj.my_rank()` but should call `exchange_obj.get_size()`. The else branch (lines 208-209) correctly calls `Get_rank()` and `Get_size()`. Copy-paste error. Causes SDFG cache UID collisions in multi-rank DaCe builds. ```python my_rank = exchange_obj.my_rank() # line 205 - correct mpi_size = exchange_obj.my_rank() # line 206 - BUG: should be .get_size() ``` ### H2. Variable `js` used outside its defining loop scope (stale value) **File:** `model/common/src/icon4py/model/common/interpolation/interpolation_fields.py:1277` **Status:** VERIFIED VALID (2025-05-04: fixed by https://github.com/C2SM/icon4py/pull/1198) After a `for js in range(lsq_dim_stencil)` loop, `js` retains its final value (`lsq_dim_stencil - 1`). Line 1277 uses this stale `js` to index `z_lsq_mat_c[jc, js, ...]`. If `lsq_dim_stencil != lsq_dim_c`, this overwrites an unrelated row. If `lsq_dim_stencil > lsq_dim_c`, an IndexError occurs. ### H3. `compute_wgtfac_e` domain has identical start and end for EdgeDim (empty domain) **File:** `model/common/src/icon4py/model/common/metrics/metrics_factory.py:634-638` **Status:** UNCERTAIN - likely bug (2025-05-04: not a bug, start and end implicitly extracted internally) ```python domain={ dims.EdgeDim: ( edge_domain(h_grid.Zone.LOCAL), edge_domain(h_grid.Zone.LOCAL), # same as start - empty domain ), ``` Other providers use `(Zone.LOCAL, Zone.END)`. Combined with `do_exchange=False`, halo edges never get correct `wgtfac_e` values. Asymmetric with `wgtfac_c` which uses full domain. ### H4. Copy-paste: `d2dexdz2_fac2_mc` populated from `D2DEXDZ2_FAC1_MC` **File:** `model/standalone_driver/src/icon4py/model/standalone_driver/driver_utils.py:289` **Status:** VERIFIED VALID (2025-05-04: fixed in https://github.com/C2SM/icon4py/pull/1225) ```python d2dexdz2_fac1_mc=metrics_field_source.get(metrics_attributes.D2DEXDZ2_FAC1_MC), # correct d2dexdz2_fac2_mc=metrics_field_source.get(metrics_attributes.D2DEXDZ2_FAC1_MC), # BUG: should be D2DEXDZ2_FAC2_MC ``` Both coefficients get FAC1 data. Downstream physics computations using `d2dexdz2_fac2_mc` produce incorrect second-order exner pressure gradient terms. ### H5. Typo in serialbox field name: `"z_gradd_exner"` instead of `"z_gradh_exner"` **File:** `model/testing/src/icon4py/model/testing/serialbox.py:1645` **Status:** VERIFIED VALID (2025-05-04: can't fix immediately, bug in icon fortran serialization) ```python def z_gradh_exner(self): return self._get_field("z_gradd_exner", dims.EdgeDim, dims.KDim) # "dd" not "dh" ``` Three other savepoint classes correctly use `"z_gradh_exner"`. This causes a runtime `SerialboxError` when this savepoint is accessed. ### H6. `dual_normal_vert_y` uses wrong geometry attribute (normal vs tangent) **File:** `model/standalone_driver/src/icon4py/model/standalone_driver/driver_utils.py:200-201` **Status:** VERIFIED VALID (2025-05-04: fixed in https://github.com/C2SM/icon4py/pull/1226) ```python dual_normal_vert_x=geometry_field_source.get(geometry_meta.EDGE_TANGENT_VERTEX_U), # correct dual_normal_vert_y=geometry_field_source.get(geometry_meta.EDGE_NORMAL_VERTEX_V), # BUG: should be EDGE_TANGENT_VERTEX_V ``` Three independent test sources confirm the correct mapping is `EDGE_TANGENT_VERTEX_V`. The y-component receives primal normal data instead of dual tangent data, corrupting tangential velocity reconstruction at vertices. Same bug appears in `test_benchmark_solve_nonhydro.py:97` and `test_benchmark_diffusion.py:104`. ### H7. Test assertion is dead code (tuple expression instead of assert) **File:** `model/common/tests/common/decomposition/unit_tests/test_halo.py:244-247` **Status:** VERIFIED (2025-05-04: fixed in https://github.com/C2SM/icon4py/pull/1234) ```python else: ( neighbor_index_full_grid[k_] == offset_full_grid[i][k], f"failed to map [{offset_full_grid[i]}] to local: [{local_offset[i]}]", ) ``` Parenthesized expression evaluates to a discarded `(bool, str)` tuple. Test always passes regardless of mapping correctness. Should be an `assert` statement. ### H8. Unreachable assertions inside `pytest.raises` blocks (multiple locations) **Files:** - `model/common/tests/common/states/unit_tests/test_factory.py:216-218` - `model/common/tests/common/states/unit_tests/test_factory.py:300-302` - `model/common/tests/common/interpolation/unit_tests/test_interpolation_factory.py:99-101` - `model/common/tests/common/grid/unit_tests/test_icon.py:128-130,153-155` - `model/common/tests/common/grid/unit_tests/test_geometry.py:51-54` - `model/common/tests/common/grid/unit_tests/test_grid_manager.py:349-356` (2025-05-05: fixed in https://github.com/C2SM/icon4py/pull/1237) Pattern: `assert` or `e.match()` placed after the exception-raising line inside `with pytest.raises(...)`, so it never executes. Error message content is never verified. ### H9. Missing `assert` in grid connectivity test **File:** `model/common/tests/common/grid/unit_tests/test_grid_manager.py:281-282` **Status:** VERIFIED (2025-05-06 fixed in https://github.com/C2SM/icon4py/pull/1242) ```python np.allclose(e2c2e_table[start_index:, :], serialized_e2c2e[start_index:, :]) np.allclose(e2c2eO_table[start_index:, :], serialized_e2c2eO[start_index:, :]) ``` `np.allclose(...)` returns a boolean that is silently discarded. Should be `assert np.allclose(...)`. ### H10. Nox session name mismatch **File:** `noxfile.py:22` **Status:** VERIFIED (2025-05-06 fixed in https://github.com/C2SM/icon4py/pull/1243) ```python nox.options.sessions = ["test_model", "test_bindings_and_tools"] # ... def test_tools_and_bindings(session: nox.Session, datatest: bool) -> None: ``` Default session `"test_bindings_and_tools"` does not match function name `"test_tools_and_bindings"`. Running `nox` without explicit sessions fails. ### H11. muphys pyproject.toml license points to non-existent file **File:** `model/atmosphere/subgrid_scale_physics/muphys/pyproject.toml:33` **Status:** VERIFIED (2025-05-06 fixed in https://github.com/C2SM/icon4py/pull/1244) ```toml license = {file = "BSD-3 License"} ``` File `"BSD-3 License"` does not exist. All other packages use `license = {text = "BSD-3 License"}` (inline text). Causes build failure. --- ## MEDIUM Severity ### M1. `dims` entries missing trailing comma (not tuples) **Files:** - `model/common/src/icon4py/model/common/states/metadata.py:145,153` - `model/common/src/icon4py/model/common/metrics/metrics_attributes.py:446,454,462` `dims=(dims.EdgeDim)` evaluates to just `dims.EdgeDim`, not `(dims.EdgeDim,)`. Other entries correctly use trailing commas. ### M2. `decomposition.is_distributed()` only checks `CellDim` halo levels **File:** `model/common/src/icon4py/model/common/decomposition/definitions.py:178` **Status:** Design concern (2025-05-06 @msimberg this is not a concern, cells is the primary thing to decompose and always has halo cells) If only edges or vertices have halo levels but cells do not, returns `False` incorrectly. ### M3. Orchestration dtype strides hardcoded for exactly 2 dimensions **File:** `model/common/src/icon4py/model/common/orchestration/dtypes.py:99-101` The loop dynamically creates stride symbols for `range(len(dims_))` but lines 99-101 hardcode exactly 2 stride entries. Breaks for 1D or 3D+ fields. ### M4. PPM boundary slope formula may not match Fortran reference **File:** `model/atmosphere/advection/src/.../stencils/compute_ppm_slope.py:42` At the bottom boundary, `p_cellhgt_mc_now` is used where the Fortran reference typically substitutes `p_cellhgt_mc_now(Koff[-1])`. May produce incorrect PPM slopes at the bottom boundary. ### M5. `eps` parameter declared but unused in PPM list variant **File:** `model/atmosphere/advection/src/.../stencils/prepare_numerical_quadrature_list_for_cubic_reconstruction.py:57` The sibling function correctly uses `eps` to clamp the area. The "list" variant omits clamping, risking division-by-zero downstream. ### M6. `GraupelInput.load` return type annotated as `-> None` instead of `-> GraupelInput` **File:** `model/atmosphere/subgrid_scale_physics/muphys/src/.../driver/common.py:101` Mypy incorrectly infers the return type, potentially masking bugs in callers. ### M7. Misleading copy-paste comments in graupel sedimentation **File:** `model/atmosphere/subgrid_scale_physics/microphysics/src/.../stencils/graupel_stencils.py:363,383` Comment says "Prevent terminal fall speed of **snow**" but code applies to **rain** (line 363) and **graupel** (line 383). ### M8. Comment `# tg <= tg:` is a copy-paste error **File:** `model/atmosphere/subgrid_scale_physics/microphysics/src/.../stencils/graupel_stencils.py:332` Should describe homogeneous freezing condition. Suggests the branch was not carefully verified. ### M9. `total_flux` asymmetry: above-ground includes `ice_flux`, at-ground does not **File:** `model/atmosphere/subgrid_scale_physics/microphysics/src/.../stencils/graupel_stencils.py:1142,1255` Undocumented asymmetry in ice flux handling between above-ground and at-ground. May be intentional but could be an oversight causing latent heat nudging to miss surface ice precipitation. ### M10. Redundant computation in `_deposition_factor` **File:** `model/atmosphere/subgrid_scale_physics/muphys/src/.../core/properties.py:76-77` Variable `x` computed on line 76, then the identical expression is written out again in the `return` on line 77 instead of using `x`. Maintenance hazard: if either copy changes independently, computation silently diverges. ### M11. Wrong error message referencing wrong parameter names **File:** `model/atmosphere/dycore/src/.../solve_nonhydro.py:261` Error message says `_max_nudging_coefficient` and `scaled_max_nudging_coefficient` but actual parameters are `_nudge_max_coeff` and `max_nudging_coefficient`. ### M12. Dual conflicting license headers **File:** `model/atmosphere/dycore/src/.../stencils/compute_cell_diagnostics_for_dycore.py:1-20` Lines 1-7 carry BSD-3-Clause; lines 9-20 carry GPL-3.0-or-later. Copy-paste error creating legal ambiguity. ### M13. Misleading variable name: `exner_new_wp` actually holds `theta_v` **File:** `model/atmosphere/dycore/src/.../stencils/update_density_exner_wind.py:31` ```python exner_new_wp = theta_v_now + dtime * grf_tend_thv # name says "exner", value is theta_v ``` Intentional ICON aliasing (Fortran reuses the same array) but actively misleading in the Python version. ### M14. Variable name typo: `virutal_temperature` instead of `virtual_temperature` **File:** `model/driver/src/icon4py/model/driver/testcases/utils.py:250` ### M15. Help text typo: "gtfn_cpu or gtfn_cpu" instead of "gtfn_cpu or gtfn_gpu" **File:** `model/driver/src/icon4py/model/driver/icon4py_driver.py:540` ### M16. `cached_property` named `backend` shadows dataclass field `backend` **File:** `model/driver/src/icon4py/model/driver/icon4py_configuration.py:47-49` Dead code with latent infinite recursion if the instance attribute is ever deleted. ### M17. `tach.toml` underdeclares `standalone_driver` dependencies **File:** `tach.toml:60-61` Declares only `icon4py.model.common` but standalone_driver actually imports from `dycore` and `diffusion`. Running `tach check` would flag these. ### M18. `model/atmosphere/advection` missing from root `pyproject.toml` `pythonpath` **File:** `pyproject.toml:247-258` All 10 other workspace members are listed. Pytest cannot find advection source via pythonpath. ### M19. Nox parametrize passes string instead of list **File:** `noxfile.py:165` ```python @nox.parametrize("selection", "basic") ``` Creates 5 sessions (one per character `b`, `a`, `s`, `i`, `c`) instead of 1 session with `"basic"`. Should be `@nox.parametrize("selection", ["basic"])`. ### M20. `scripts/tests.py` tuple append flattening bug **File:** `scripts/tests.py:288,291` ```python errors += (path, fixture) # extends with individual elements, not a tuple ``` Produces flat list `[path1, fixture1, ...]` instead of `[(path1, fixture1), ...]`. Error count via `len(errors)` reports 2x the actual number. ### M21. muphys `pyproject.toml` author entry split into two objects **File:** `model/atmosphere/subgrid_scale_physics/muphys/pyproject.toml:8-9` ```toml authors = [{email = "gridtools@cscs.ch"}, {name = "ETH Zurich"}] ``` Should be a single object: `[{email = "gridtools@cscs.ch", name = "ETH Zurich"}]`. ### M22. Wrong units docstring for `dtime` parameter **File:** `model/atmosphere/dycore/src/.../velocity_advection.py:256` Says `[m s-1]` (velocity units) instead of `[s]` (time units). ### M23. `wgtfacq_c_dsl` index semantics confusing with `z_aux_c` **File:** `model/common/src/icon4py/model/common/metrics/compute_weight_factors.py:148-149` Column indices are permuted relative to the original computation, making the code hard to verify against Fortran. --- ## LOW Severity ### L1. Typo in function name: `timestemp` instead of `timestep` **File:** `model/atmosphere/diffusion/src/.../diffusion_utils.py:80` Both definition and call site use the same misspelling, so no runtime error. ### L2. Comment typos - `model/atmosphere/diffusion/src/.../diffusion.py:451` - "hat activates" should be "that activates" - `model/atmosphere/diffusion/src/.../diffusion.py:114` - "shar" should be "shear" - `model/atmosphere/diffusion/src/.../diffusion_states.py:79` - "Nudgeing coeffients" should be "Nudging coefficients" ### L3. `1.0 / 6.3` instead of `10.0 / 63.0` in Jablonowski-Williamson topography **File:** `model/common/src/icon4py/model/common/initialization/topography.py:36` Same numerical value but harder to verify against reference paper. ### L4. `HorizontalFluxLimiter` uses `@abstractmethod` without inheriting `ABC` **File:** `model/atmosphere/advection/src/.../advection_horizontal.py:56-66` `@abstractmethod` is a no-op without `ABC` base class. ### L5. `maximum(0.1 * rhodz_in, rhodz_in)` does not prevent negative density **File:** `model/atmosphere/advection/src/.../stencils/apply_density_increment.py:32` When `rhodz_in < 0`, result is `0.1 * rhodz_in` which is still negative. ### L6. Vertical line slope substitution uses coordinate value, not epsilon **File:** `model/atmosphere/advection/src/.../stencils/prepare_ffsl_flux_area_patches_list.py:100-101` `d1 = where(d1 != 0.0, d1, line1_p2_lon)` replaces zero with an arbitrary coordinate value. ### L7. Floating-point comparison for integer condition in PPM fractional flux **File:** `model/atmosphere/advection/src/.../stencils/compute_ppm4gpu_fractional_flux.py:82` ### L8. Constant name typo: `AIR_KINEMETIC_VISCOSITY` should be `AIR_KINEMATIC_VISCOSITY` **File:** `model/atmosphere/subgrid_scale_physics/microphysics/src/.../microphysics_constants.py:101` ### L9. Class name typo: `SnowInterceptParametererization` should be `SnowInterceptParameterization` **File:** `model/atmosphere/subgrid_scale_physics/microphysics/src/.../microphysics_options.py:23` ### L10. Inconsistent precision literals in GT4Py field operators **Files:** - `model/atmosphere/subgrid_scale_physics/microphysics/src/.../stencils/microphyiscal_processes.py:34,1087,1131` - `model/atmosphere/subgrid_scale_physics/microphysics/src/.../stencils/saturation_adjustment_stencils.py:140` Bare Python floats (`5.0`, `1e-20`, `1850.0`, `1.0`) instead of `wpfloat(...)`. Could cause precision differences when `wpfloat` is `float32`. ### L11. Variable `chlp` reused with different semantics in same scope **File:** `model/atmosphere/subgrid_scale_physics/microphysics/src/.../stencils/graupel_stencils.py:245,508` ### L12. Misleading comment on `B1` coefficient **File:** `model/atmosphere/subgrid_scale_physics/muphys/src/.../core/transitions.py:366` ### L13. Precision inconsistency in `interpolate_vt_to_interface_edges` **File:** `model/atmosphere/dycore/src/.../stencils/interpolate_vt_to_interface_edges.py:24` First multiply uses `vpfloat` inputs then casts to `wpfloat`, instead of using already-cast `wpfloat` versions. ### L14. Test class named `TestRainToGraupel1` in file `test_rain_to_graupel_2.py` **File:** `model/atmosphere/subgrid_scale_physics/muphys/tests/muphys/stencil_tests/test_rain_to_graupel_2.py:18` ### L15. Test variable `virtual_temperature` holds `temperature()` data **File:** `model/common/tests/common/diagnostic_calculations/unit_tests/test_diagnostic_calculations.py:205` ### L16. Parametrized test parameters never used (marked with TODOs) **File:** `model/common/tests/common/diagnostic_calculations/unit_tests/test_diagnostic_calculations.py:244-257` ### L17. Missing Python 3.11 classifier in muphys `pyproject.toml` **File:** `model/atmosphere/subgrid_scale_physics/muphys/pyproject.toml:19` ### L18. `cuda11` extra in bindings not accessible from root **File:** `bindings/pyproject.toml:47` ### L19. Type annotation mismatch: `process_props` default `None` without `| None` **File:** `model/driver/src/icon4py/model/driver/initialization_utils.py:544` ### L20. `__post_init__` parameter type inconsistent with `InitVar` annotation **File:** `model/standalone_driver/src/icon4py/model/standalone_driver/driver_states.py:142` ### L21. Potential shape mismatch in `init_w` when halos present **File:** `model/standalone_driver/src/icon4py/model/standalone_driver/testcases/utils.py:200-209` ### L22. Potential missing guard for `nshift_total >= n_levels - 1` **File:** `model/common/src/icon4py/model/common/grid/vertical.py:270` --- ## Review Tasks - [x] model/common/src - shared code (grid, math, utils, states, decomposition, etc.) - [x] model/atmosphere/advection/src - [x] model/atmosphere/diffusion/src - [x] model/atmosphere/dycore/src - [x] model/atmosphere/subgrid_scale_physics/microphysics/src - [x] model/atmosphere/subgrid_scale_physics/muphys/src - [x] model/driver/src and standalone_driver/src - [x] model/testing/src - [x] tools/src - [x] Tests across all packages (test correctness, not coverage) - [x] Config files (pyproject.toml, tach.toml, noxfile.py, scripts) ## Summary | Severity | Count | |----------|-------| | HIGH | 11 | | MEDIUM | 23 | | LOW | 22 | | **Total**| **56** | ### Top priorities (fix immediately) 1. **H4** - `d2dexdz2_fac2_mc` gets wrong data (copy-paste, one-line fix) 2. **H6** - `dual_normal_vert_y` uses wrong geometry attribute (one-line fix) 3. **H5** - `z_gradd_exner` typo causes runtime error (one-line fix) 4. **H1** - `mpi_size = my_rank()` in orchestration decorator (one-line fix) 5. **H2** - stale `js` variable corrupts LSQ interpolation matrix 6. **H10** - nox default session name mismatch (one-line fix) 7. **H11** - muphys license file reference broken (one-line fix) 7. **H7-H9** - test assertions are dead code / missing assert

Syntax	Example	Reference
# Header	Header	基本排版
- Unordered List	Unordered List
1. Ordered List	Ordered List
- [ ] Todo List	Todo List
> Blockquote	Blockquote
Bold font	Bold font
Italics font	Italics font
~~Strikethrough~~	~~Strikethrough~~
19^th^	19^th
H~2~O	H₂O
++Inserted text++	Inserted text
==Marked text==	Marked text
[link text](https:// "title")	Link
![image alt](https:// "title")	Image
`Code`	`Code`	在筆記中貼入程式碼
```javascript var i = 0; ```	`var i = 0;`
:smile:		Emoji list
{%youtube youtube_id %}	Externals
$L^aT_eX$	L^aT_eX
:::info This is a alert area. :::	This is a alert area.