NCAR / ccpp-physics

Collection of physics parameterizations compliant with the Common Community Physics Package (CCPP) framework
Other
60 stars 146 forks source link

Noah MP must use host model constants instead of its own ones #582

Open climbfuji opened 3 years ago

climbfuji commented 3 years ago

Noah MP in several places uses its own constants, e.g. for the gravitational acceleration. This needs to be fixed to receive/set constants via the argument list. See https://github.com/NCAR/ccpp-physics/pull/575.

barlage commented 3 years ago

Is there an example, say in rrtmgp (since it is a separate repo), where this is done? Just browsing the land models, all of them define their own constants. I'm not saying this is the proper thing to do, but it seems we need a generalized approach to make this happen.

grantfirl commented 3 years ago

I wrote this in #575, but it belongs here:

"To be clear, I'm talking about a subset of the physical constants defined here. In particular, grav, sb, vkc, tfrz, hsub, hvap, hfus, cwat, cice, cpair, rair, rw, and denh2o are all defined by existing CCPP host models. And by "going through the CCPP", I'm referring to physics using physical constants defined by the host model so that all physics using them can use a consistent set. Otherwise, if every scheme has its own value of gravity, specific heats, etc., this introduces a lot of inconsistencies. If other users of the NoahMP repository require the scheme to define its own physical constants, then we need to figure out a solution that works for everybody. One solution would be to define a NoahMP specific constants module. If using the CCPP, during the init phase of the scheme, we could pass in the physical constants from the host and set the constants in the NoahMP-specific constants module inside. If NOT using the CCPP, one could use pre-defined values in the NoahMP-specific module and not overwrite them in the init phase."

There are lots of schemes that define their own constants, and, in my opinion, I don't think that is a problem as long as the constants they are defining will ONLY ever be used within that scheme. If there is a chance that another scheme might use a given constant, then I think that it should be defined by the host model and passed in.

grantfirl commented 3 years ago

@climbfuji has previously provided the following example from the UGWP scheme to others dealing with the same thing:

"For an example, you can look at the ugwp code in the current NCAR ccpp-physics master repository, too:

https://github.com/NCAR/ccpp-physics/blob/master/physics/cires_ugwpv1_initialize.F90 https://github.com/NCAR/ccpp-physics/blob/master/physics/ugwpv1_gsldrag.F90 "

This provides an example of what I described in the previous post (although it is not in its own repository): using a scheme-specific constants module and setting them inside the init phase. If the scheme is simple enough (e.g. not very many subroutines), the constants can also just be passed in the run phase and propagated through the internal scheme code where necessary. For schemes with complex organization (like the UGWP scheme above), following the scheme-specific constants module method requires the fewest code/interface changes.

grantfirl commented 3 years ago

@dustinswales, do you have any input on this issue since you're maintaining your own repo for RRTMGP? That is, do other non-CCPP users of your repo expect physical constants (those that are potentially common to other schemes) to be defined internally? If so, how do you handle it?

barlage commented 3 years ago

It is mentioned here: https://github.com/earth-system-radiation/rte-rrtmgp/blob/33c8a984c17cf41be5d4c2928242e1b4239bfc40/rrtmgp/mo_rrtmgp_constants.F90#L12

but it would be good to know if this is used in CCPP.

barlage commented 3 years ago

This seems like a good method:

https://github.com/earth-system-radiation/rte-rrtmgp/blob/33c8a984c17cf41be5d4c2928242e1b4239bfc40/rrtmgp/mo_rrtmgp_constants.F90#L57

grantfirl commented 3 years ago

Yep, it looks like RRTMGP has the functionality that I described, but I'm not actually seeing anywhere in the CCPP interface to RRTMGP where mo_rrtmgp_constants%constants_init is actually called. But, that is a good example, if it were actually called.

@dustinswales It looks like constants are passed in to various RRTMGP-based interstitial schemes, but I'm not seeing that, for example, gravity internal to RRTMGP is set to the CCPP-provided one. Does this mean that RRTMGP is currently using its own value too? Please enlighten us when you get a chance.

dustinswales commented 3 years ago

@grantfirl You are correct. GP is using its internal definitions. The init_constants() allows users to override the internal values. I should add a call to this init_constants during GP initialization, at least for gravity. @RobertPincus

RobertPincus commented 3 years ago

For context, RRTMGP uses four constants: Avogadro's number, the molecular weights of dry air and water vapor, and gravitational acceleration. These are used to compute the number of molecules per area from pressure and gas concentrations (i.e. to determine the opacity of the atmosphere). Making them consistent with the host model is a fine idea but maybe more important when we start using the UFS for exoplanets.

grantfirl commented 3 years ago

Thanks, gentlemen (@dustinswales and @robertpincus). I have added an issue (#583) when you get a chance.

climbfuji commented 3 years ago

Thanks everyone, especially @grantfirl for continuing this discussion while I was out on the trails. I think we have a good way forward for schemes with and without their own repository.