add doc string on the combined stencils on velocity advection #707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cscs-ci run default |
|
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:
In case your change might affect downstream icon-exclaim, please consider running
For more detailed information please look at CI in the EXCLAIM universe. |
jcanton
reviewed
Apr 3, 2025
| Args: | ||
| - normal_wind_advective_tendency: horizontal advection tendency of the normal wind | ||
| - vn: normal wind at edges | ||
| - horizontal_kinetic_energy_at_edges_on_model_levels: horizontal kinematic energy at edges on model levels |
There was a problem hiding this comment.
Suggested change
| - horizontal_kinetic_energy_at_edges_on_model_levels: horizontal kinematic energy at edges on model levels | |
| - horizontal_kinetic_energy_at_edges_on_model_levels: horizontal kinetic energy at edges on model levels |
jcanton
reviewed
Apr 3, 2025
| - normal_wind_advective_tendency: horizontal advection tendency of the normal wind | ||
| - vn: normal wind at edges | ||
| - horizontal_kinetic_energy_at_edges_on_model_levels: horizontal kinematic energy at edges on model levels | ||
| - horizontal_kinetic_energy_at_cells_on_model_levels: horizontal kinematic energy at cell centers on model levels |
There was a problem hiding this comment.
Suggested change
| - horizontal_kinetic_energy_at_cells_on_model_levels: horizontal kinematic energy at cell centers on model levels | |
| - horizontal_kinetic_energy_at_cells_on_model_levels: horizontal kinetic energy at cell centers on model levels |
jcanton
reviewed
Apr 3, 2025
| - coriolis_frequency: coriolis frequency parameter | ||
| - contravariant_corrected_w_at_cells_on_model_levels: contravariant-corrected vertical velocity at model levels | ||
| - vn_on_half_levels: normal wind on half levels | ||
| - geofac_rot: interpolation field |
There was a problem hiding this comment.
Suggested change
| - geofac_rot: interpolation field | |
| - geofac_rot: metrics field for rotor computations |
jcanton
reviewed
Apr 3, 2025
| - vn_on_half_levels: normal wind on half levels | ||
| - geofac_rot: interpolation field | ||
| - coeff_gradekin: metrics field/coefficient for the gradient of kinematic energy | ||
| - c_lin_e: interpolation field/linear interpolation coefficients from cells to edges |
There was a problem hiding this comment.
Suggested change
| - c_lin_e: interpolation field/linear interpolation coefficients from cells to edges | |
| - c_lin_e: metrics field for linear interpolation from cells to edges |
jcanton
reviewed
Apr 3, 2025
| - geofac_rot: interpolation field | ||
| - coeff_gradekin: metrics field/coefficient for the gradient of kinematic energy | ||
| - c_lin_e: interpolation field/linear interpolation coefficients from cells to edges | ||
| - ddqz_z_full_e: metrics field |
There was a problem hiding this comment.
Suggested change
| - ddqz_z_full_e: metrics field | |
| - ddqz_z_full_e: metrics field equal to vertical spacing |
jcanton
reviewed
Apr 3, 2025
| - area_edge: area associated with each edge | ||
| - tangent_orientation: orientation of the edge with respect to the grid | ||
| - inv_primal_edge_length: inverse primal edge length | ||
| - geofac_grdiv: interpolation field = |
There was a problem hiding this comment.
Suggested change
| - geofac_grdiv: interpolation field = | |
| - geofac_grdiv: metrics field used to compute the gradient of a divergence (of vn) |
jcanton
reviewed
Apr 3, 2025
| - scalfac_exdiff: scalar factor for external diffusion | ||
| - d_time: time step | ||
| - levelmask: mask for valid vertical levels | ||
| - nlev: total number of vertical levels |
There was a problem hiding this comment.
Suggested change
| - nlev: total number of vertical levels | |
| - nlev: number of (full/model) vertical levels |
jcanton
reviewed
Apr 3, 2025
| - d_time: time step | ||
| - levelmask: mask for valid vertical levels | ||
| - nlev: total number of vertical levels | ||
| - nrdmax: vertical index where damping ends. |
There was a problem hiding this comment.
Suggested change
| - nrdmax: vertical index where damping ends. | |
| - nrdmax: vertical index where damping ends |
jcanton
reviewed
Apr 3, 2025
| - nlev: total number of vertical levels | ||
| - nrdmax: vertical index where damping ends. | ||
| - start_vertex_lateral_boundary_level_2: start index of lateral boundary level 2 on vertex | ||
| - end_vertex_halo: ebd index of halo on vertex |
There was a problem hiding this comment.
Suggested change
| - end_vertex_halo: ebd index of halo on vertex | |
| - end_vertex_halo: end index of halo on vertex |
jcanton
reviewed
Apr 3, 2025
| - vertical_end: end index in the vertical domain | ||
|
|
||
| Returns: | ||
| - normal_wind_advective_tendency |
There was a problem hiding this comment.
Suggested change
| - normal_wind_advective_tendency | |
| - normal_wind_advective_tendency: horizontal advection tendency of the normal wind | |
jcanton
reviewed
Apr 3, 2025
|
|
||
| Args: | ||
| - contravariant_corrected_w_at_cells_on_model_levels: contravariant-corrected vertical velocity at model levels | ||
| - vertical_wind_advective_tendency: vertical advection tendency |
There was a problem hiding this comment.
Suggested change
| - vertical_wind_advective_tendency: vertical advection tendency | |
| - vertical_wind_advective_tendency: vertical advection tendency of the vertical wind |
same as you did for
- normal_wind_advective_tendency: horizontal advection tendency of the normal wind
jcanton
reviewed
Apr 3, 2025
| - w: vertical wind at cell centers | ||
| - contravariant_corrected_w_at_cells_on_half_levels: contravariant-corrected vertical velocity at cells on half levels | ||
| - horizontal_advection_of_w_at_edges_on_half_levels: horizontal advection for vertical velocity at edges on half levels | ||
| - coeff1_dwdz: metrics field (first coefficient for vertical derivative of 'w') |
There was a problem hiding this comment.
Suggested change
| - coeff1_dwdz: metrics field (first coefficient for vertical derivative of 'w') | |
| - coeff1_dwdz: metrics field (first coefficient for vertical derivative of vertical wind) |
(I prefer w, but you called it "vertical wind" everywhere else)
jcanton
reviewed
Apr 3, 2025
| - contravariant_corrected_w_at_cells_on_half_levels: contravariant-corrected vertical velocity at cells on half levels | ||
| - horizontal_advection_of_w_at_edges_on_half_levels: horizontal advection for vertical velocity at edges on half levels | ||
| - coeff1_dwdz: metrics field (first coefficient for vertical derivative of 'w') | ||
| - coeff2_dwdz: metrics field (second coefficient for vertical derivative of 'w') |
There was a problem hiding this comment.
Suggested change
| - coeff2_dwdz: metrics field (second coefficient for vertical derivative of 'w') | |
| - coeff2_dwdz: metrics field (second coefficient for vertical derivative of vertical wind) |
jcanton
reviewed
Apr 3, 2025
| Formerly known as fused_velocity_advection_stencil_8_to_13_predictor. | ||
|
|
||
| This interpolates horizontal kinetic energy from edges to cells on models levels | ||
| and compute the contravariant correction term on half levels |
There was a problem hiding this comment.
Suggested change
| and compute the contravariant correction term on half levels | |
| and compute the contravariant correction term on half levels. |
jcanton
reviewed
Apr 3, 2025
|
|
||
| This interpolates horizontal kinetic energy from edges to cells on models levels | ||
| and compute the contravariant correction term on half levels | ||
| It also computes the vertical velocity with the contravariant correction term |
There was a problem hiding this comment.
Suggested change
| It also computes the vertical velocity with the contravariant correction term | |
| It also computes the vertical velocity with the contravariant correction term. |
jcanton
reviewed
Apr 3, 2025
|
|
||
| This computes the derived horizontal wind components, kinetic energy, horizontal advection of the vertical velocity, | ||
| and the contravariant correction. | ||
| It also extrapolates vertical velocity at the top level |
There was a problem hiding this comment.
Suggested change
| It also extrapolates vertical velocity at the top level | |
| It also extrapolates vertical velocity at the top level. |
jcanton
reviewed
Apr 3, 2025
| Agrs: | ||
| - tangential_wind: tangential wind at model levels | ||
| - tangential_wind_on_half_levels: tangential wind interpolated to half levels | ||
| - vn_on_half_levels: normal wind on half levels |
There was a problem hiding this comment.
Suggested change
| - vn_on_half_levels: normal wind on half levels | |
| - vn_on_half_levels: normal wind interpolated to half levels |
jcanton
reviewed
Apr 3, 2025
| - halo_1: end index of halo on edges | ||
|
|
||
| Returns: | ||
| - tangential_wind |
There was a problem hiding this comment.
Suggested change
| - tangential_wind | |
| - tangential_wind |
jcanton
reviewed
Apr 3, 2025
There was a problem hiding this comment.
why is that stuff duplicated?
@functools.cached_property
def nrdmax(self) -> gtx.int32:
"""Vertical index where damping ends."""
return self.end_index_of_damping_layer
@functools.cached_property
def end_index_of_damping_layer(self) -> gtx.int32:
"""Vertical index where damping ends."""
return self.index(Domain(dims.KDim, Zone.DAMPING))
There was a problem hiding this comment.
There was a time when we decided we kept both, one with the old ICON name and the other one with the new easier-to-understand name.
Now it is a good time we remove nrdmax in my opinion because we have decided to rename variables. And add that this new name corresponds to nrdmax.
|
mostly looks good to me, but I'm looking forward to when we will finally make this properly automated with a dict or something so you don't have to re-write or copy/paste the same description in multiple places with the risk of different descriptions :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding the doc string on the combined stencil on velocity advection:
The updated documentation includes: