Skip to content

Another attempt at 1 to 13 #762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Another attempt at 1 to 13 #762

wants to merge 14 commits into from

Conversation

muellch
Copy link
Contributor

@muellch muellch commented May 27, 2025

Re-writing the stencil 1_to_13 to better suit dace optimization pipeline, while keeping it as readable as possible for scientists.
Understanding all of the changes in this PR requires good understanding of the ICON Fortran code.

@muellch muellch requested a review from OngChia May 28, 2025 11:51
from icon4py.model.common.math.derivative import _compute_first_vertical_derivative_at_cells
from icon4py.model.common.type_alias import vpfloat, wpfloat


horzpres_discr_type: Final = HorizontalPressureDiscretizationType()


@field_operator
def _initialize_zero_if_limited_area(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need this to set these two variables to zero from (start_cell_lateral_boundary, start_cell_lateral_boundary_level_3)? They are initialized to be zero fields when instantiating the granule, and the cells in (start_cell_lateral_boundary, start_cell_lateral_boundary_level_3) is never recomputed?

end_cell_halo=end_cell_halo,
end_cell_halo_level_2=end_cell_halo_level_2,
horizontal_start=0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like the removal of horizontal_start, horizontal_end, and if possible vertical_start and vertical_end. Otherwise, it is very confusing with the combination of many starting/ending index names with horizontal_start and horizontal_end. I would suggest we also rename vertical_start and vertical_end to model_top_index and surface_level_index or num_levels_plus_one (you can think of a nicer name), respectively. And maybe we should adopt it in other stencils as well.

(just putting a reminder here that the docstring also needs to be edited whenever other things are finished)

@@ -523,6 +483,21 @@ def compute_perturbed_quantities_and_interpolation(
},
)

_compute_perturbation_of_rho_and_theta(
Copy link
Contributor

@OngChia OngChia May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small suggestion. Maybe I would prefer to put this field operator call directly right after the field operator _compute_perturbed_quantities_and_interpolation for better readability instead of here. And maybe rename it as _compute_perturbation_of_rho_and_theta_at_extra_halo_cells

z_theta_v_pr_ic=perturbed_theta_v_at_cells_on_half_levels,
theta_v_ic=theta_v_at_cells_on_half_levels,
z_th_ddz_exner_c=pressure_buoyancy_acceleration_at_cells_on_half_levels,
dims.KDim >= 1,
Copy link
Contributor

@OngChia OngChia May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion to increase readability. How about we remove this field operator _compute_pressure_gradient_and_perturbed_rho_and_potential_temperatures and, just like what you did above for rho_at_cells_on_half_levels, we call _interpolate_cell_field_to_half_levels_vp and _interpolate_cell_field_to_half_levels_wp directly here to calculate perturbed_theta_v_at_cells_on_half_levels and theta_v_at_cells_on_half_levels? Or we can group all these interpolation_to_half level computation into one field operator.

I think pressure_gradient in the field operator's name is not accurate enough for the variable pressure_buoyancy_acceleration_at_cells_on_half_levels (originally z_th_ddz_exner_c in ICON) because it also includes the buoyancy term. I would create a new field operator called _calculate_pressure_buoyancy_acceleration_at_cells_on_half_levels (also needed in corrector step) just to calculate pressure_buoyancy_acceleration_at_cells_on_half_levels.
Something like this

(
        perturbed_rho_at_cells_on_half_levels,
        perturbed_theta_v_at_cells_on_half_levels,
        perturbed_perturbed_theta_v_at_cells_on_half_levels
) = concat_where(
        dims.KDim >=1,
        _interpolate_rho_and_theta_v_to_half_levels(
        wgtfac_c=wgtfac_c,
        current_rho=current_rho,
        current_theta_v=current_theta_v,
        perturbed_theta_v_at_cells_on_model_levels=perturbed_theta_v_at_cells_on_model_levels,
)

pressure_buoyancy_acceleration_at_cells_on_half_levels = concat_where(
        dims.KDim >=1,
        _calculate_pressure_buoyancy_acceleration_at_cells_on_half_levels(
                exner_w_explicit_weight_parameter,
                theta_v_at_cells_on_half_levels,
                perturbed_exner_at_cells_on_model_levels,
                perturbed_theta_v_at_cells_on_half_levels,
                ddqz_z_half,
                ddz_of_reference_exner_at_cells_on_half_levels,
        )
)

Copy link

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark-bencher

To run tests and benchmarks with the DaCe backend you can use:

  • cscs-ci run dace

To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:

  • cscs-ci run extra

For more detailed information please look at CI in the EXCLAIM universe.

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.

2 participants