Skip to content
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

Port z ifc #564

Open
wants to merge 70 commits into
base: main
Choose a base branch
from
Open

Port z ifc #564

wants to merge 70 commits into from

Conversation

jcanton
Copy link
Contributor

@jcanton jcanton commented Oct 4, 2024

currenlty waiting on this
GridTools/gt4py#1746

@jcanton
Copy link
Contributor Author

jcanton commented Nov 18, 2024

cscs-ci run default

@jcanton
Copy link
Contributor Author

jcanton commented Nov 18, 2024

launch jenkins spack

jcanton and others added 7 commits November 18, 2024 14:49
* move dycore stencils to sub packages stencils
---------

Co-authored-by: Enrique González Paredes <[email protected]>
* filter out unused connectivities
* remove diffusion_instance fixture
* update member blacklist in Diffusion.orchestration_uid
model/common/src/icon4py/model/common/grid/topography.py Outdated Show resolved Hide resolved
model/common/src/icon4py/model/common/grid/topography.py Outdated Show resolved Hide resolved
model/common/src/icon4py/model/common/grid/topography.py Outdated Show resolved Hide resolved
model/common/src/icon4py/model/common/grid/topography.py Outdated Show resolved Hide resolved
model/common/src/icon4py/model/common/grid/vertical.py Outdated Show resolved Hide resolved
f"Model top is too low. But num_levels, {vertical_config.num_levels}, <= 6. "
)

return gtx.as_field((dims.CellDim, dims.KDim), z3d_i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reflect this in the code, by splitting into functions with good names? At least I would split what you do in init_vert_coordinate into 2 functions, one which computes the height (down to here) and another one that does this layer thickness thing that comes from here on. Then the init_vert_coordinate calls both of them.

def init_vertical_height_field(...)-> Field[Dims[CellDim, KDim], wpfloat]:
    z_3d =  compute_height_from_idealized_flat_heights_and_topology(vct_a, topology):
    z_3d = correct_layer_thickness(...)
    z_3d = check_flatness_of_flat_layer()
    return z_3d

This is some pseudo code. (I don't even know what would be a proper name for the vct_a compared to the z_ifc...) The idea that I want to transport is, suppose I read icon4py code then depending on what interests me I am happy with seeing somewhere compute_vertical_height_field, I know what is happening in the function and go on (supposedly that the function does this and only this). If I am interested in the height field, then I can jump into the function and I see that 1. it takes contribution from topology and vct_a, 2. there is something to take into account of the thickness of the resulting layers, 3. the previously (or configured) flat layer needs to still be consistent. May that is all that interests me. So I go on. Only if I need to know how the thickness of the minimal layers is determined I need to jump further down into the 2nd of these functions. Otherwise the reader does not need to care of the gritty details of indices and fields and dimension and etc.

Good code transports meaning and it is always organized in different levels of abstraction or details. Such that a reader can ignore the details if he does not need them.

And you write code once and you read it 1000 times...

This is one of the reasons why ICON code is so hard to understand that there is (almost) no level in the code above the most detailed one. If I want to see what algorithm they use they throw 3 nested loops at me. From these I need to make sense what relations ship they are computing.

model/common/tests/grid_tests/test_topography.py Outdated Show resolved Hide resolved
model/common/tests/grid_tests/test_vertical.py Outdated Show resolved Hide resolved
model/common/tests/grid_tests/test_vertical.py Outdated Show resolved Hide resolved
@jcanton
Copy link
Contributor Author

jcanton commented Nov 20, 2024

cscs-ci run default

@jcanton
Copy link
Contributor Author

jcanton commented Nov 20, 2024

launch jenkins spack

Copy link

Mandatory Tests

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

  • cscs-ci run default
  • launch jenkins spack

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark

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

  • cscs-ci run dace

In case your change might affect downstream icon-exclaim, please consider running

  • launch jenkins icon

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

@jcanton
Copy link
Contributor Author

jcanton commented Nov 21, 2024

cscs-ci run default

@jcanton
Copy link
Contributor Author

jcanton commented Nov 21, 2024

launch jenkins spack

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.

6 participants