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

Substepping outside dycore, refactor grid_wrapper, add mean_cell_area and vol_flx_ic #683

Merged
merged 7 commits into from
Mar 4, 2025

Conversation

muellch
Copy link
Contributor

@muellch muellch commented Feb 26, 2025

Model:

  • Pull substepping outside of the dycore
  • fix bounds in predictor
    Fortran wrappers:
  • Extract grid_wrapper instead of one grid_init per diffusion/dycore granule
  • Add mean_cell_are and vol_flx_ic to interface
  • Change name max_nudge_coeff to indicate it's the ICON scaled one that is passed to the bindings
  • Adjust nflat_gradp to Python indexing in the bindings.

@havogt havogt changed the title Pull substepping outside dycore, fix bounds Pull substepping outside dycore, fix bounds, refactor grid_wrapper, add mean_cell_area Feb 28, 2025
@havogt
Copy link
Contributor

havogt commented Feb 28, 2025

cscs-ci run default

@havogt havogt changed the title Pull substepping outside dycore, fix bounds, refactor grid_wrapper, add mean_cell_area Substepping outside dycore, refactor grid_wrapper, add mean_cell_area and vol_flx_ic Feb 28, 2025
@havogt havogt requested a review from halungge February 28, 2025 10:13
vertical_end=gtx.int32(self._vertical_params.nflat_gradp + 1),
offset_provider=self._grid.offset_providers,
)
if self._vertical_params.nflatlev < gtx.int32(self._vertical_params.nflat_gradp + 1):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it happen that nflatlev>= nflat_gradp + 1 ? Shouldn't the condition rather be checked when computing these values at initialization? The other questions is does gt4py crash for a "negative" or "empty" domain?

Copy link
Contributor

@havogt havogt Feb 28, 2025

Choose a reason for hiding this comment

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

Yes, it crashed for an empty domain. We should probably fix that on the GT4Py side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I would suggest so. Anyway, we can leave this as a workaround for now. Can you open an issue in GT4Py?

Copy link
Contributor

Choose a reason for hiding this comment

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

On a second thought, I am not sure if it should be legal, because that could also hide user errors. At least GT4Py should warn that it doesn't execute a program. Here is the issue GridTools/gt4py#1891

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I agree it could hide errors, but there are valid cases for it: for example if you run something like APE every layer is flat, so if you try to compute something from some layer up to the first flat layer, that might collapse (what probably happens in the example here) or in the horizontal if you compute something only on the boundaries, but you have a global model, then there are no boundaries. The user would then have to always do an extra
if domain-is-not-empty check what the if condition above essentially is.

@@ -256,6 +245,7 @@ def test_dycore_wrapper_granule_inputs(

# PrepAdvection
vn_traj = sp.vn_traj()
vol_flx_ic = data_alloc.zero_field(icon_grid, dims.CellDim, dims.KDim) # TODO sp.vol_flx_ic()
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, you wanted the vol_flx_ic in the init savepoint? I think I only added it to the exit... shame on me...

@havogt
Copy link
Contributor

havogt commented Feb 28, 2025

cscs-ci run default

@havogt
Copy link
Contributor

havogt commented Feb 28, 2025

cscs-ci run default

@halungge halungge self-requested a review February 28, 2025 15:42
@havogt
Copy link
Contributor

havogt commented Feb 28, 2025

cscs-ci run default

Copy link

github-actions bot commented Mar 4, 2025

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.

@havogt
Copy link
Contributor

havogt commented Mar 4, 2025

tests passed in #687

@havogt havogt merged commit dab637c into main Mar 4, 2025
2 checks passed
@havogt havogt deleted the debuging_dycore_granule branch March 4, 2025 08:43
@havogt havogt mentioned this pull request Mar 4, 2025
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.

None yet

3 participants