Verify global model #315

halungge · 2023-11-17T08:25:17Z

Add verification for a global model experiment (exclaim_ape_R02B04)

makes the experiment parametrizable in the tests: Configuration and serialized data are chosen according to the experiment name.
Diffusion: adapt flow and stencil for apply_diffusion_to_w_and_compute_horizontal_gradients_for_turbulence.py to allow for shear_type (see icon-exclaim PR)
NonHydrostaticConfig now uses ICON default values as default parameters
add calculation of k_start_moist
add debug log statements to SolveNonHydro

Further fixes :

(FIX) remove dependency of model/common on model/atmosphere/diffusion by moving construction of the diffusion specific types from serialbox data out of common/test_utils
(FIX) fix duplicated paths in data_handling.py
invert arguments to dallclose such that the reference values comes in second place see
remove some unused code from dycore/state_utils and common/test_utils/serialbox_utils.py, VectorTuple type

add module docstring

- add verification diffusion.init for global experiment

halungge · 2023-11-17T08:26:15Z

cscs-ci run

…le for experiment

… APE

enable tests for both experiments in test_diffusion.py and test_velocity_advection.py remove caching in common/grid/base.py

change offset bound in velocity_advection.py

… in second place: https://numpy.org/doc/stable/reference/generated/numpy.isclose.html#numpy.isclose

halungge · 2023-12-05T12:38:25Z

cscs-ci run

halungge · 2023-12-05T12:39:18Z

launch jenkins spack

halungge · 2023-12-05T17:51:13Z

cscs-ci run

halungge · 2023-12-05T17:51:30Z

launch jenkins spack

halungge · 2023-12-05T18:26:12Z

cscs-ci run

abishekg7 · 2023-12-05T19:12:40Z

launch jenkins spack

abishekg7 · 2023-12-05T19:12:48Z

launch jenkins icon

abishekg7

Seems like a lot of work! Looks good overall. I mostly have minor comments, but we do need to pay some attention to tolerances.

abishekg7 · 2023-12-13T14:03:02Z

model/atmosphere/diffusion/tests/diffusion_tests/test_diffusion.py

-def test_smagorinski_factor_for_diffusion_type_4(r04b09_diffusion_config):
-    config = r04b09_diffusion_config
+def test_smagorinski_factor_for_diffusion_type_4(experiment):
+    config = construct_config(experiment, ndyn_substeps=5)


is the ndyn_substeps hard-coding here okay?

what is hardcoded? I don't understand what you mean?

I thought ndyn_substeps=5 should go into the part where the rest of the namelist options are present. But if it's a temporary design thing, that's fine.

abishekg7 · 2023-12-13T14:08:20Z

model/atmosphere/diffusion/tests/diffusion_tests/test_diffusion.py

-        interpolation_state=interpolation_state,
-        edge_params=edge_geometry,
-        cell_params=cell_geometry,
+    assert dallclose(diffusion.enh_smag_fac.asnumpy(), savepoint.enh_smag_fac(), rtol=1e-7)


these tolerances seem a little high. Is it also the case with blue line?

abishekg7 · 2023-12-13T14:17:27Z

model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/nh_solve/solve_nonhydro.py

@@ -1834,9 +1900,11 @@ def run_corrector_step(
            vertical_end=self.grid.num_levels,
            offset_provider={},
        )
+        # TODO (magdalena) stencil_60 is missing here?


what we are doing about stencil 60? Will it be in a later PR?

Yes, that is why I added the TODO. Did not want to fix this in the same PR.

abishekg7 · 2023-12-13T14:18:12Z

model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/state_utils/diagnostic_state.py

-    exner_incr: Field[[EdgeDim, KDim], float]  # exner increment [- ]
+    exner_incr: Field[[CellDim, KDim], float]  # exner increment [- ]
+
+    exner_dyn_incr: Field[[CellDim, KDim], float] = None  # exner pressure dynamics increment


probably need to add a test for IAU mode

There is no IAU mode in greenline. There is no way we are going to do any data assimilation soon in greenline, so I don't see what we should test there.

Right. i meant that we don't have any data assimilation tests at all (blue or greenlines), but we have the stencils. We should think eventually about adding tests for this. Not in this PR ofc.

abishekg7 · 2023-12-13T14:36:55Z

model/atmosphere/dycore/tests/dycore_tests/test_solve_nonhydro.py

        diagnostic_state_nh.mass_fl_e.asnumpy(),
-        rtol=1e-10,
+        savepoint_nonhydro_exit.mass_fl_e().asnumpy(),
+        rtol=5e-7,  # TODO (magdalena) was rtol=1e-10 for local experiment only


this seems weird. there are no prescribed tolerances for this stencil in the blueline

yes, I agree its weird. I have not been able to locate any possible bug, which of course does not mean its not there.

let's add an issue to track this, if we want to get it merged.

abishekg7 · 2023-12-13T14:37:45Z

model/atmosphere/dycore/tests/dycore_tests/test_solve_nonhydro.py

        prep_adv.vn_traj.asnumpy(),
-        rtol=1e-10,
+        savepoint_nonhydro_exit.vn_traj().asnumpy(),
+        rtol=5e-7,  # TODO (magdalena) was rtol=1e-10 for local experiment only


these two tolerances are questionable too. different from blueline

abishekg7 · 2023-12-13T14:41:55Z

model/common/src/icon4py/model/common/test_utils/serialbox_utils.py

    def zd_diffcoef(self):
        return self._get_field("zd_diffcoef", CellDim, KDim)

+    @optionally_registered


what does this do?

It accounts for those fields being present in the global run, because they do not even get allocated there, so they are not registered in the serialbox savepoint.

okay thanks!

abishekg7 · 2023-12-13T14:47:44Z

model/atmosphere/dycore/tests/dycore_tests/utils.py

+
+from icon4py.model.atmosphere.dycore.nh_solve.solve_nonhydro import NonHydrostaticConfig
+
+


I guess an alternative method would be to define the entire config (diffusion + nonhydro + ...) for LAM or APE experiments, and then individual configs will pick up the config they need. But not sure about pros and cons.

yes. Where this should go is: some thing ( a class what ever) reads the configuration from file does necessary consistency checks and divides it up into the configuration bits needed for the individual components.

abishekg7 · 2023-12-13T14:49:48Z

...e/diffusion/stencils/apply_diffusion_to_w_and_compute_horizontal_gradients_for_turbulence.py

@@ -99,12 +104,13 @@ def apply_diffusion_to_w_and_compute_horizontal_gradients_for_turbulance(
    vertical_start: int32,
    vertical_end: int32,
 ):
-    _apply_diffusion_to_w_and_compute_horizontal_gradients_for_turbulance(
+    _apply_diffusion_to_w_and_compute_horizontal_gradients_for_turbulence(


thanks for changing this!

abishekg7

LGTM. You can merge after syncing/testing.

halungge · 2023-12-15T10:42:54Z

cscs-ci run

halungge · 2023-12-15T11:04:42Z

launch jenkins spack

remove empty diagnostic_state.py remov l_open_ubc from solve_nonhydro.py

halungge · 2023-12-15T13:06:26Z

cscs-ci run

halungge · 2023-12-15T13:06:35Z

launch jenkins spack

github-actions · 2023-12-15T14:08:22Z

Mandatory Tests

Please make sure you run these tests via comment before you merge!

cscs-ci run
launch jenkins spack

Optional Tests

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.

halungge · 2023-12-15T14:27:24Z

cscs-ci run

halungge · 2023-12-15T14:27:42Z

launch jenkins spack

halungge added 12 commits November 9, 2023 11:43

remove unusde fields from metrics savepoint

5e7af70

remove dependency diffusion in common/test_utils

c3acfd8

use system epsilon

853f5c9

remove unused fields from serialbox_utils.py

8afbb4e

fix imports in io_utils.py

0237b20

remove unused typed definition

1b8271b

add module docstring

fix wrong import in test_diffusion_states.py

e0e8398

Merge branch 'main' into verify_global_model

acfc1cc

use dallclose in diffusion

050cbc9

diffusion: make choice of configuration flexible in tests

8516c51

- pass experiment name a argument to datapath

85583a9

- add verification diffusion.init for global experiment

WIP diffusion single_step test for exclaim APE

877f199

halungge added 17 commits November 17, 2023 11:19

WIP (1) make velocity advection and solve nonhdyro tests parametrizab…

d745e5d

…le for experiment

WIP (2) make velocity advection and solve nonhdyro tests parametrizab…

e2d3583

…le for experiment

Merge branch 'main' into verify_global_model

fcbf9ca

WIP (1) make solve_nonhydro.py choose configuration dynamically

d7bd316

run test_solve_nonhydro.py::test_run_nonhydro_single_step for exclaim…

cf5a1a0

… APE

add experiment fixture to common and driver

c23d7e5

run on specified backend for test_diffusion_utils.py

0b14265

enable tests for both experiments in test_diffusion.py and test_velocity_advection.py remove caching in common/grid/base.py

enable both experiments in test_solve_nonhydro.py

fdbfe63

change offset bound in velocity_advection.py

WIP: fix assert in test_diffusion.py

085ad42

merge main

f0dfaa1

test_velocity_advection.py: for both experiments

a4206ca

test_diffusion.py: for both experiments

ac21490

fix tests: test_diffusion_utils.py, test_vertical.py

e0f4298

test_solve_nonhydro.py: invert args in dallclose, reference should be…

c3df5aa

… in second place: https://numpy.org/doc/stable/reference/generated/numpy.isclose.html#numpy.isclose

add missing config parameters in Diffusion config

d1efb33

fix imports

eed4bc3

- make data loading fixture dependent

68c8b77

cleanups

ef8be6d

halungge marked this pull request as ready for review December 5, 2023 12:50

halungge requested a review from abishekg7 December 5, 2023 12:50

halungge added 2 commits December 5, 2023 15:18

fix import of experiment fixture in model/common

b54f476

fix nsubsteps in multistep test

b96b7b4

pre-commit

3149f36

halungge mentioned this pull request Dec 6, 2023

Fix greenline dependencies #327

Merged

merge main

c720607

abishekg7 reviewed Dec 13, 2023

View reviewed changes

halungge mentioned this pull request Dec 14, 2023

tolerances for green line global experiment verificaiton #340

Open

abishekg7 approved these changes Dec 14, 2023

View reviewed changes

merge main, resolve conflicts

23e78e4

fix merge errors:

7425855

remove empty diagnostic_state.py remov l_open_ubc from solve_nonhydro.py

fix merge errors: test_velocity_advection.py

f5fd1e7

halungge merged commit 6e153b5 into main Dec 15, 2023
5 checks passed

halungge deleted the verify_global_model branch December 12, 2024 07:00


		from icon4py.model.atmosphere.dycore.nh_solve.solve_nonhydro import NonHydrostaticConfig

Verify global model #315

Verify global model #315

Conversation

halungge commented Nov 17, 2023 • edited Loading

halungge commented Nov 17, 2023

halungge commented Dec 5, 2023

halungge commented Dec 5, 2023

halungge commented Dec 5, 2023

halungge commented Dec 5, 2023

halungge commented Dec 5, 2023

abishekg7 commented Dec 5, 2023

abishekg7 commented Dec 5, 2023

abishekg7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abishekg7 left a comment

Choose a reason for hiding this comment

halungge commented Dec 15, 2023

halungge commented Dec 15, 2023

halungge commented Dec 15, 2023

halungge commented Dec 15, 2023

github-actions bot commented Dec 15, 2023

halungge commented Dec 15, 2023

halungge commented Dec 15, 2023

halungge commented Nov 17, 2023 •

edited

Loading