-
Notifications
You must be signed in to change notification settings - Fork 8
Update serialized data #1004
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?
Update serialized data #1004
Conversation
msimberg
left a comment
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.
Thanks @jcanton!
I'm a bit confused how this passes in CI, but is it because the github workflows don't run datatests? At least I couldn't find any quickly. I suppose you didn't re-serialize all the experiments, and if that's the case I would expect the test_geometry.py tests to fail for spherical grids (which is ok, as discussed; in that case we make the comparison to serialized data conditional on the geometry type in the tests).
|
I didn't trigger the CI ( |
…lement run_serialization script for MPI job management
| @@ -0,0 +1,300 @@ | |||
| #!/usr/bin/env python3 | |||
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.
Would be nice to use firecrest I guess, but as a first step this is nice!
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.
that's outside my paygrade ;-)
but I'll be happy to learn from whoever ports it to firecrest
| description="R02B04, small global grid, default grid used in ICON testing, origin of this file is unclear (source = icon-dev)", | ||
| sizes={"cell": 20480, "vertex": 10242, "edge": 30720}, | ||
| kind=GridKind.GLOBAL, | ||
| shape=icon_grid.GridShape( |
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.
nice!
|
|
||
|
|
||
| class GridKind(enum.Enum): | ||
| REGIONAL = "REGIONAL" |
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 don't use the REGIONAL property anywhere?
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.
nah, it's a leftover from who knows what, never used anywhere...
also grid root and level -> global_num_cells -> mean_cell_area can be removed, but will do the cleanup in a separate PR as that's purely cleaning
| # ====================================== | ||
| MPI_RANKS: list[int] = [1, 2, 4] | ||
|
|
||
| EXPERIMENTS = [ |
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.
read from icon4py.model.testing.definitions?
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.
and iterate over the Experiments class?
or modify that to be a list?
scripts/run_serialization.py
Outdated
| total_ranks = mpi_ranks + reserved_ranks | ||
| original = script_path.read_text() | ||
|
|
||
| updated = original |
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.
btw, this does not create a copy (which seems to be the intention), simply an alias. on the other hand re.sub does not modify the input string, so it would be safe to replace this (and the first call to re.sub) with
updated = re.sub(..., original, ...)
|
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. |
Add cartesian coordinates to serialized data (sync with https://github.com/C2SM/icon-exclaim/pull/406 AND UPDATE CI DATA)
this allows for testing of the
math/helpers.py::geographical_to_cartesian_on_xyzfunctionsit will also allow for MPI tests on torus #692 which need serialized decomposed grid cartesian coordinates