Physics driver l2

[yiluchen1066]

No description provided.

[msimberg]

This is not really a review, but more questions and comments about minor and major things. Mostly for my own understanding and reminder to revisit as the PR evolves, but adding the comments in case someone else has thought about them already and has answers.

[msimberg]

Question for myself:I suppose this is one of the critical interfaces, though the signature could likely be tightened to something resembling state: (Something?)State, time_step: datetime.datetime? Or the whole class should inherit from a protocol.

[msimberg]

Minor note as well that may need broader discussion: If this is indeed the protocol, my preference is to give this a name rather than using the generic __call__ operator, but I also know we already use __call__ extensively, so may be me still not being used to __call__ being used everywhere.

[msimberg]

And further thinking out loud: Could this be based on a more generic TendencyAdaptor? The muphys-specific parts seem quite minimal and the interesting logic seems to be the _to_tendency parts.

[jcanton]

and time_step should be datetime.timedelta if we decide to use timedeltas for parametrizing (as opposed to the integer time steps)

[jcanton]

correction: it should be RelativeTime either from standalone_driver or time.py depending on whether #1353 is merged before this one or not

[msimberg]

I guess this is in seconds? I wonder if we can add a bit more type information to this (something from datetime?) and just convert to the correct units as late as possible (i.e. when passing into gt4py programs).

[jcanton]

after a separate discussion with @msimberg we came up with this for the standalone driver:
https://github.com/C2SM/icon4py/pull/1304/changes#diff-1374141d85d171d3fa2c5711b656072708d65ff1080beaf0cdeedcc64f105bd7R37

should we use it here too?

[jcanton]

todo after you fix the merge:
I had not noticed this change from t to te in the other PR.
I think these two should be handled with if use_icon_nwp like everything else, i.e.:

if use_icon_nwp:
    # return the temperature updated by the final saturation adjustment (as the
    # Fortran does), consistent with the adjusted qv/qc
    return te, Q(v=qve, c=qce, r=q.r, s=q.s, i=q.i, g=q.g), pflx, pr, ps, pi, pg, pre
else:
    return t, Q(v=qve, c=qce, r=q.r, s=q.s, i=q.i, g=q.g), pflx, pr, ps, pi, pg, pre

just with the corrected use_icon_nwp or match case consistent with the other changes.

[jcanton]

correction: it should be RelativeTime either from standalone_driver or time.py depending on whether #1353 is merged before this one or not

[jcanton]

more comments but haven't finished reviewing yet...

[jcanton]

Suggested change

	if use_aes_graupel:
	# return the temperature updated by the final saturation adjustment (as the
	# Fortran does), consistent with the adjusted qv/qc
	return te, Q(v=qve, c=qce, r=q.r, s=q.s, i=q.i, g=q.g), pflx, pr, ps, pi, pg, pre
	else:
	return t, Q(v=qve, c=qce, r=q.r, s=q.s, i=q.i, g=q.g), pflx, pr, ps, pi, pg, pre

[msimberg]

These are starting to be nitpicky comments, so this is definitely getting close.

The Component/State discussion (i.e. the protocols not being ready) is my biggest concern, but we leave that out from this PR.

[jcanton]

more comments I had forgotten about

[jcanton]

what I mean is that instead of calling _require 15-20 times in the code, you can just verify once at the setup of the component, or the first time it is called, that all tracers necessary are active by doing something like all(SPECIES in TracerState.active_fields()) (with proper syntax), makes sense?

[OngChia]

Instead of diagnosing all those diagnostic variables used in microphysics or each process, we need a better strategy. We need to check whether the process wants to use the most up-to-date variables or just use the values at the beginning of the time loop (after dycore).
For reference, in NWP, fast physics always uses the variables updated by preceding physics process, while slow physics uses the values after all fast physics processes have been called.
On the other hand, AES physics always uses the values at the beginning of each time loop and returns tendencies or change rate.

[yiluchen1066]

we need to think about a better strategy here, in the second pr

[OngChia]

Nothing wrong, It is just very unfortunate that inp stands for ice-nucleating particles in microphysics community. So, this variable name becomes confusing for microphysics researchers if they happen to look at the code.

[OngChia]

usually in ICON or other models, there is k level above which microphysics is turned off (usually for manual performance tuning, may be for stability issues in some rare cases). In ICON, it is kstart_moist. How about passing in vertical params? Could you check whether we can remove this manual performance tuning so that we always run the microphysics for all levels?

[yiluchen1066]

I will check this

[OngChia]

Ignore my comment here. This is just for myself.
I am surprised that AES uses a fixed qnc. Check why.

[OngChia]

Maybe not in this PR. We need to make this optional if we are following aes strategy. I remember that aes updates all prognostic variables at the very end of all physics calls.

[github-actions]

When developing, you can test your changes on CSCS CI before merge with the default pipeline: cscs-ci run default. This will run a default subset of tests.

You can pass options to override pipeline variables, for example:

• cscs-ci run default;BACKENDS=gtfn_cpu;LEVELS=unit
• cscs-ci run default;MODEL_SUBPACKAGES=common:standalone_driver;SESSIONS=model
  Avoid running the pipeline for all tests when you are developing.

Available options are:

• SESSIONS: model, model_mpi, or tools (correspond to nox sessions)
• MODEL_SUBSETS: datatest, basic, or stencils (correspond to nox session selections)
• MODEL_SUBPACKAGES: subpackages for non-MPI tests (last component, e.g. diffusion, standalone_driver)
• MODEL_MPI_SUBPACKAGES: subpackages for MPI tests (as above)
• BACKENDS: backends
• GRIDS: grids for stencil tests (simple, icon_regional, or icon_global)
• LEVELS: testing level for non-stencil tests (unit or integration)

For each option, all can be used as a shorthand for all possible values of that variable, e.g. LEVELS=all.

See scripts/python/generate_ci_pipeline.py and noxfile.py for available values for each option.

The all pipeline can be run with cscs-ci run all. This will run all icon4py tests in CSCS CI which can be expensive. This pipeline runs on a schedule on main, and can be run when extensive validation is needed (e.g. before releases).

Merging

Once your PR is approved and ready for merging, add it to the merge queue. The merge CSCS CI pipeline will run automatically on the merge-queue branch and must pass before the PR is merged. A dummy merge check will be triggered on the PR itself since it's required to add a PR to the merge queue.

Optional Tests

To run benchmarks you can use:

• cscs-ci run benchmark-bencher

For more detailed information please look at CI in the EXCLAIM universe.