-
Notifications
You must be signed in to change notification settings - Fork 7
Better torus support #965
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?
Better torus support #965
Conversation
* main: Update DaCe version (#962) blueline: move vertical grid to grid_wrapper, add flat_height (#956) CI testdata moved to `cwci02` project (#960) Bump actions/checkout from 5 to 6 (#955) Test fixes for metrics fields (#949) Remove pytest-deadfixtures from test dependency (#950) Revert "Add debug info to stencil tests" Add debug info to stencil tests
…nto better-torus-support-metrics * origin/better-torus-support-geometry: Apply suggestion from @msimberg
…his but it doesn't work)
(r,b) are not used anymore to check whether we're dealing with torus: we have GeometryType for that, so they can be whatever value for torus and we can remove a bunch of | Nones
model/common/src/icon4py/model/common/interpolation/interpolation_fields.py
Show resolved
Hide resolved
|
cscs-ci run default |
| return ( | ||
| tangent_x, | ||
| tangent_y, | ||
| tangent_z, | ||
| tangent_u, | ||
| tangent_v, | ||
| normal_x, | ||
| normal_y, | ||
| normal_z, | ||
| normal_u, | ||
| normal_v, | ||
| ) |
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.
here why we cannot just return the components on x and y. I think u and v are just the copy of them
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 could but it does not make a difference in terms of memory usage to do it here (and doing it in other places makes it less readable IMO)
model/common/src/icon4py/model/common/interpolation/interpolation_fields.py
Outdated
Show resolved
Hide resolved
model/common/src/icon4py/model/common/interpolation/interpolation_fields.py
Outdated
Show resolved
Hide resolved
model/common/src/icon4py/model/common/interpolation/interpolation_fields.py
Outdated
Show resolved
Hide resolved
model/common/src/icon4py/model/common/interpolation/interpolation_fields.py
Outdated
Show resolved
Hide resolved
model/common/src/icon4py/model/common/interpolation/interpolation_fields.py
Outdated
Show resolved
Hide resolved
|
cscs-ci run default |
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.
I'm a bit blind to my own changes, but @jcanton improvements look very good to me.
I only have a few minor requests for changes.
model/common/src/icon4py/model/common/interpolation/rbf_interpolation.py
Outdated
Show resolved
Hide resolved
|
cscs-ci run default |
|
cscs-ci run default |
| """ | ||
| return distance_on_edges_torus( | ||
| vertex_x(E2C2V[2]), |
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.
Suddenly, I am thinking if is it better to define far indices in icon.py as a enum class or global constants or something else instead of putting a hardcoded number here, such as
E2C2V_NEIGHBOR_INDEX1 = 0
E2C2V_NEIGHBOR_INDEX2 = 1
E2C2V_FAR_INDEX1 = 2
E2C2V_FAR_INDEX2 = 3
(I do not remember whether gt4py allows that or not, and I put this comment because it seems that the figure in the docstring of arc_distance_of_far_edges_in_diamond seems to be inconsistent with the actual indices, leading to confusion. If we have a universal definition, then it will be clearer)
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 fixed the docstsring, nice catch!
I tried some ways (constants, enum class) to move the indexes to some named variable but gt4py doesn't like it, and I cannot find any other field_operator that does this so I am guessing this is not supported, right @havogt ?
model/common/src/icon4py/model/common/interpolation/interpolation_factory.py
Show resolved
Hide resolved
|
cscs-ci run default |
|
cscs-ci run default |
|
cscs-ci run default |
model/common/tests/common/metrics/unit_tests/test_metric_fields.py
Outdated
Show resolved
Hide resolved
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.
|
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. |
|
cscs-ci run default |
Based upon #953 #954 and #957 by @msimberg
Adds torus support to geometry, RBF and interpolation.
Also adds torus grids to all datatests