-
Notifications
You must be signed in to change notification settings - Fork 7
Halo construction #540
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
base: main
Are you sure you want to change the base?
Halo construction #540
Conversation
extract conventional ownership assignment pass connectivities explicitly in order to avoid sparse matrix 0 - based index business. (preliminary fix)
- read data fields from grid file (only selected indices)
fix imports in grid_manager
move test to for local grid to test_grid_manager.py
# Conflicts: # model/common/src/icon4py/model/common/grid/grid_manager.py
… standalone driver
| transformation: IndexTransformation, | ||
| grid_file: pathlib.Path | str, | ||
| config: v_grid.VerticalGridConfig, # TODO(halungge): remove to separate vertical and horizontal grid | ||
| num_levels: int, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get rid of the num_levels here, it is now only here because we have it in the BaseGrid which historically comes from the fact that we had it in the SimpleMesh
|
|
||
| # TODO(msimberg): Compute these in GridGeometry once FieldProviders can produce scalars. | ||
| # This will also allow easier handling once grids are distributed. | ||
| # TODO (@halungge): use global reduction in geometry.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we should not read them from the gridfile, as they are not set in all grid files, rather move th computation of the fields below (edge_length, cell_area ... back to the geometry.py and compute the means there by global reduction.
That could also simplyfy this GlobalGridParams struct a lot.
|
|
||
|
|
||
| def _refinement_level_placed_with_halo(domain: h_grid.Domain) -> int: | ||
| """There is a speciality in the setup of the ICON halos: generally halo points are located at the end of the arrays after all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The numbers used here are really determined by reverse engineering in ICON. There is no reference in any documentation to that afaik. ICON places these not boundary-halo points there, the reason to me is mysterious and I think it it worth trying to run without that.
In any case, it is only relevant for LAM models.
|
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:
To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
| backend: gtx_typing.Backend | None, | ||
| grid: test_defs.GridDescription, | ||
| ) -> None: | ||
| if test_utils.is_dace(backend): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear to me what happens here: The test runs when running it in isolation, but does not when all tests in the module are run. In fact it makes other fail as well. The test_halo_neighbor_access_e2c shows the same behavior. I don't know whether this is a matter of the halo setup (maybe still some ordering is incorrect?) or an issue with dace caching. Candidates to be investigated:
- both stencil functions used in these test use direct neighborhood access.
- the connectivities do have skip values
- caching? I am not entirely sure what is cached when by which backend...
| backend: gtx_typing.Backend | None, | ||
| grid: test_defs.GridDescription, | ||
| ) -> None: | ||
| if test_utils.is_dace(backend): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
|
cscs-ci run default |
Decompose (global) grid file:
pymetisto decompose the global grid (cells) intonpatchesOmissions:
LAM grids need to be investigated further:
start_indexandend_indexnot in the halo construction.the number of halo lines (in terms of cells) is hardcoded to 2, that could be made a parameter.
Not sure it all runs on GPU correctly... most probably there are some
numpycupyissues to fix.