-
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
Microphysics package with saturation adjustment #522
Conversation
cscs-ci run default |
launch jenkins spack |
cscs-ci run default |
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 a bit unsure on how to go on in relation with this PR concerning the interface there not the ADR. Should we try to make any "new" component into such an interface or is it better to keep the current "granule" style and then adapt them together with changing the driver?
I thought it would be good to take the first option, that is why I opened that PR in the first place, but I come to think that the second option is maybe better.
model/atmosphere/subgrid_scale_physics/microphysics/pyproject.toml
Outdated
Show resolved
Hide resolved
model/atmosphere/subgrid_scale_physics/microphysics/tests/test_saturation_adjusment.py
Outdated
Show resolved
Hide resolved
|
||
|
||
# TODO (Chia Rui): Refactor this class when there is consensus in gt4py team about the best way to express compile-time constants | ||
class ConvectTables(FrozenNamespace): |
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.
would that be a ConvectionTable
? Or what does the Convect
stand for?
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 was named mo_convect_tables
in ICON (convective adjustment table). But I just realized that it has became mo_lookup_tables_constants
after I pull the latest branch... I now rename it to SaturatedPressureConstants
in icon4py because, as far as I understand, it should only contain constants related to saturated pressure, which are used by saturation adjustment and microphysics components. Some constants such as rv and rd in SaturatedPressureConstants
will be moved back to common/constants.py when the coding style of compile-time constants suggested by gt4py is fixed.
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 think Nikki was working on that: GridTools/gt4py#1615 would that already fix the issue?
...ics/src/icon4py/model/atmosphere/subgrid_scale_physics/microphysics/saturation_adjustment.py
Show resolved
Hide resolved
cscs-ci run default |
Perhaps I will leave the saturaton_adjustment component as it is now in this PR. And I found that the input and output of graupel microphysics have just slightly more, but similar, arguments than saturation adjustment. The progress there is good now. I will open another PR after graupel merge to make these two components follow the interface protocol and more seriously think about integration into the driver. How is this plan? |
I have also added a few more diagnostic computations after the saturation adjustment. |
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.
As you have an entire new package the license update that we did (we switched from GPL to BSD 3 together with gt4py) is not completely smooth for you. I think you need to
- merge the current main branch (that will update all the other packages)
- update the pre-commit version in the pyproject of the
microphysics
package. - copy the LICENSE file from another package to your microphyscis package
- re-run pre-commit
running pre-commit should hopefully update the license headers in all files inside your package.
@@ -12,7 +12,7 @@ | |||
|
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.
Is this a new stencil or is this one used in the blueline? If blueline, did you check for the dtype
in 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.
This stencil is called in subroutine perform_nh_stepping
before the time loop starts in mo_nh_stepping.f90
. It is only called once throughout the execution of the icon program. The stencil name has now become compute_exner_pert
(previously it was called init_exner_pr
). What it does is simply subtracts one variable from another variable, which should belong to a more generic stencil. Because it involves variables in DiagnosticStateHydro
, I initially put it under solve_nonyhdro
package.Now, I think that it should be put somewhere else. I will put it in driver/src/test_cases/utils.py
for the time being, and I suppose in the future these kinds of generic stencils that only execute simple arithmetic operations will be created somewhere in common
.
And thanks for asking me to check the dtype
again. I made a mistake here. The correct type for exner_ref
should be vp_float
.
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 also update its name to compute_perturbed_exner
and add a docstring to the stencil citing where it comes from.
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.
Liked the new name. Thank you. I always thought exner_pr
stand for exner_pressure
and was wondering why and what the difference was to the exner
which is also exner pressure... Never guessed that _pr
stands for "perturbation"... so much for the abbreviations.... 🙈
...ics/microphysics/src/icon4py/model/atmosphere/subgrid_scale_physics/microphysics/__init__.py
Show resolved
Hide resolved
...ics/microphysics/src/icon4py/model/atmosphere/subgrid_scale_physics/microphysics/__init__.py
Outdated
Show resolved
Hide resolved
model/common/src/icon4py/model/common/states/diagnostic_state.py
Outdated
Show resolved
Hide resolved
model/atmosphere/subgrid_scale_physics/microphysics/.pre-commit-config.yaml
Outdated
Show resolved
Hide resolved
model/atmosphere/subgrid_scale_physics/microphysics/tests/__init__.py
Outdated
Show resolved
Hide resolved
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.
For my curiosity: what is actually the RBF part of this interpolation?
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 stencil uses the C2E2C2E connectivity. The cell center value is computed from values located at the 9 edges of neighboring cells based on some RBF coefficients. I put a documentation (mostly copied from ICON) here for reference.
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.
Ah so the interpolation coefficent that is used is generated by some RBF functions. But then would you agree that as the coefficients are arguments to the stencils (so any suitable interpolation coefficient, produced by whatever method) and the connectivity is just a connectivity, the stencil function itself independent of RBF?
We should maybe rename (not necessarily in this PR).
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.
Highly agreed. I will rename it in my subsequent PR. let me think of a better name (maybe something like edge_2_cell_vector_interpolation_with_c2e2c2e_coef). I added a TODO there so that I won't forget.
model/common/tests/stencil_tests/test_diagnose_surface_pressure.py
Outdated
Show resolved
Hide resolved
model/common/tests/stencil_tests/test_diagnose_surface_pressure.py
Outdated
Show resolved
Hide resolved
cscs-ci run default |
launch jenkins spack |
cscs-ci run default |
launch jenkins spack |
cscs-ci run default |
That is fine for me. |
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 left some remarks, (more discussion points...)
For me is fine to leave the component interface to a separate PR.
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:
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 |
* empty microphysics package * merge from graupel branch. Saturation adjustment component in progress * saturation adjustment component completed * introduce type_alias in saturation adjustment * embedded graupel scheme tested ok * remove graupel from this branch * add documentation * precommit and clean up * remove redundant comment in test_saturation_adjustment * change physics to subgrid_scale_physics and removed redundant code * fix wrong rename * precommit and fix documentation * undo torus temporary fix and ammend satad documentation * update installation requirement text and readme in model package * fix model install requirement * review changes * add original name in ICON for constants * add more docstring * add diagnostic computation after saturation adjustment * fix potential bugs in diagnostic calculations and add in diagnostic part following satad call * new download link for wk experiment * split into two datatests for satad, fix bug, and add todo in diagnose_temperature * precommit * fix diagnostic stencil tests * move init_exner_pr * change import for diagnostic stencils and tests * precommit * fix license issue in microphysics package * add docstring for edge_2_cell_vector_rbf_interpolation * set virtual temp in driver test cases and fix import * add todo for WK experiemnt grid * rm init_exner_pr stencil test * add satad input output attributes, and common model attributes * use the new grid domain function in satad --------- Co-authored-by: Magdalena Luz <[email protected]>
Add saturation adjustment component to icon4py. All future physics will be placed under
subgrid_scale_phyiscs
folder.run
function that returns the tendencies, conforming with the decision made in this PR Component interface #506. However, a follow-up action needs to be done in a subsequent PR to make this component fully follow the protocol.Some additional fixes are: