-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor advection stencils to use StencilTest
#336
Conversation
cscs-ci run |
cscs-ci run |
cscs-ci run default |
1 similar comment
cscs-ci run default |
cscs-ci run default |
cscs-ci run default |
1 similar comment
cscs-ci run default |
cscs-ci run benchmark |
cscs-ci run default |
cscs-ci run default |
cscs-ci run default |
cscs-ci run default |
launch jenkins spack |
launch jenkins icon |
launch jenkins spack |
cscs-ci run default |
cscs-ci run default |
launch jenkins spack |
launch jenkins icon |
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.
It looks good to me, I have added a few comments and questions, otherwise it must be good!
@@ -30,7 +30,7 @@ def _btraj_dreg_stencil_01( | |||
return lvn_sys_pos | |||
|
|||
|
|||
@program | |||
@program(grid_type="unstructured") |
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.
All dycore stencils are written like this: grid_type=GridType.UNSTRUCTURED
I imagine the two are equivalent but perhaps it would be better to be uniform with the dycore. This is a detail so I leave it up to you.
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.
Those statements should have been added in all stencils, correct? Most of the hflx_limiter_mo/pd_stencil
, hor_adv_stencil_01
, rbf_intp_edge_stencil_01
don't have 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.
I am not sure if it is best practice to do this for all stencil programs. I just applied this to the stencils which failed to execute otherwise. Asking @tehrengruber what he thinks.
PROGRAM = face_val_ppm_stencil_01 | ||
OUTPUTS = ( | ||
Output( | ||
"z_slope", refslice=(slice(None), slice(None, -1)), gtslice=(slice(None), slice(1, -1)) |
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.
This is useful thanks. If I understood correctly gtslice
is the slicing of the GridTools output and in parenthesis are the dimensions.
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.
Yes so refslice
is the slice for the reference output, and gtslice
is the slice for the gt4py output field. By default both are None
which means the whole field.
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.
You can also still use the normal syntax: OUTPUTS = ("a", "b")
but then the fields are not sliced.
z_slope.asnumpy(), | ||
) | ||
# Concatenate the NaN column to the existing array | ||
nan_column = np.full((p_face.shape[0], 2), np.nan) |
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.
Why not using the output
option and slice the two last columns of the GridTools output?
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.
Yes that makes sense, thanks for the suggestion.
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
In case your change might affect downstream icon-exclaim, please consider running
For more detailed information please look at CI in the EXCLAIM universe. |
cscs-ci run default |
launch jenkins spack |
cscs-ci run default |
Refactor half of all advection stencil to use StencilTest class, and make outputs sliceable. --------- Co-authored-by: Nina Burgdorfer <Nina.Burgdorfer@meteoswiss.ch>
Refactor advection stencils to use
StencilTest
class.