Remove IPD steps 3 and 5 (cleanup preprocessor directives)

[climbfuji]

This PR is part of the removal process of IPD from the UFS and addresses steps 3 and 5 in NOAA-EMC/fv3atm#214.

Changes:

• Remove all preprocessor directives that were introduced to "switch" between CCPP fast physics and the original fast physics implementation, alongside with the non-CCPP fast physics code.
• Replace imports of of IPD types and containers (IPD_) with the corresponding GFS types/containers (GFS_) if preprocessor directive GFS_TYPES is set (in fv3atm's top-level CMakeLists.txt)

This PR fixes #29.

Associated PRs:

#50
NCAR/ccpp-physics#547
NOAA-EMC/fv3atm#224
NOAA-EMC/NEMS#87
ufs-community/ufs-weather-model#357

For regression testing, see ufs-community/ufs-weather-model#357.

[climbfuji]

@junwang-noaa please assign reviewers when it is convenient for you, thanks.

[junwang-noaa]

I am curious what is the impact of the code changes to MULTI_GASES?

[climbfuji]

None, because it is inside the #ifndef CCPP block.

[XiaqiongZhou-NOAA]

Just wondering if ccpp is the default now, what does "ifndef ccpp" mean?

[climbfuji]

Just wondering if ccpp is the default now, what does "ifndef ccpp" mean?

Until now, we pass -DCCPP to the compiler when compiling the dycore code (and all the other model code). The PRs listed above remove this -DCCPP since it is now the only option. We thus need to make sure that only the code blocks that currently correspond to #ifdef CCPP or the #else option in #ifndef CCPP blocks are executed.

[bensonr]

The IPD data types are removed entirely from the code base. They are actually not more generic, at least not the way they are in fv3atm now, because all they do is pass through the GFS data types one by one.

The layer of indirection comes from the IPD design taking a different approach than the CCPP. The IPD approach was that each physics suite, consisting of a group of parameterizations and a driver, could be imported independently by creating an IPD-compliant translation layer. The CCPP adopted a method of aggregating parameterizations, creating interstitials when needed, and auto-generating a driver. Each design has pros and cons.

[climbfuji]

The IPD data types are removed entirely from the code base. They are actually not more generic, at least not the way they are in fv3atm now, because all they do is pass through the GFS data types one by one.
The layer of indirection comes from the IPD design taking a different approach than the CCPP. The IPD approach was that each physics suite, consisting of a group of parameterizations and a driver, could be imported independently by creating an IPD-compliant translation layer. The CCPP adopted a method of aggregating parameterizations, creating interstitials when needed, and auto-generating a driver. Each design has pros and cons.

Thanks, Rusty, and I totally agree with your assessment. Given that the UFS has decided to adopt the CCPP going forward and remove the IPD from the codebase, do you agree to make the changes I propose here or do you think we should keep the IPD data types in the model? (for what reason?)

[bensonr]

Thanks, Rusty, and I totally agree with your assessment. Given that the UFS has decided to adopt the CCPP going forward and remove the IPD from the codebase, do you agree to make the changes I propose here or do you think we should keep the IPD data types in the model? (for what reason?)

I am okay with the changes. If anyone needs the IPD bindings, the history and provenance are still in place. Also, the IPD bindings will remain available for direct reference from within the authoritative main branch (currently 'master').

[junwang-noaa]

Please understand we also need to sync back the dycore change to GFDL's central repo, from what I understanding since the dycore is shared by several host models, it is arguable not to add host model specifics in the shared code, otherwise we may have to stick to our specific dycore version from GFDL's release version. I really don't see how the discussion here can lead to "you want to keep the door open to remove CCPP again and reintroduce the IPD", such a statement is not rational to me.

…

On Tue, Jan 12, 2021 at 12:43 PM Dom Heinzeller ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tools/fv_iau_mod.F90 <#50 (comment)> : > @@ -53,8 +53,8 @@ module fv_iau_mod get_var1_double, & get_var3_r4, & get_var1_real, check_var_exists - use IPD_typedefs, only: IPD_init_type, IPD_control_type, & - kind_phys => IPD_kind_phys + use GFS_typedefs, only: GFS_init_type, GFS_control_type, & I don't know if this platform is the best way to discuss this. From the point of view of the CCPP, the host model alone defines how it stores its data. Whether this is in derived data types (GFS or IPD or whatever) doesn't matter, as long as any data that needs to get passed to the physics is "registered" with the CCPP in form of a standard name and other attributes in a metadata file. We could just keep the IPD data types in fv3atm, but I don't see why unless you want to keep the door open to remove CCPP again and reintroduce the IPD. We could also remove the GFS DDTs entirely (I don't want to do this, but just explaining) and define each variable individually (or automate this using a registry such as WRF or MPAS). We could also decide to remove the blocking strategy and define each variable as a contiguous array and CCPP will automatically pass the correct slices to the physics. No matter what, the physics (fast physics inside the dycore and traditional/slow physics outside the dycore) will run just fine. They don't care how data is stored as long as the host model allocates and initializes those variables. If you decide to port NAM physics to CCPP, then those physics will run just fine with however the data is stored, too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI7D6TORKUMX5SH3SRJIKWTSZSC5JANCNFSM4V2U5ZSQ> .

[climbfuji]

Please understand we also need to sync back the dycore change to GFDL's central repo, from what I understanding since the dycore is shared by several host models, it is arguable not to add host model specifics in the shared code, otherwise we may have to stick to our specific dycore version from GFDL's release version. I really don't see how the discussion here can lead to "you want to keep the door open to remove CCPP again and reintroduce the IPD", such a statement is not rational to me.
…

I'll reply to your various comments in one go. When you say CIME, I assume you mean CESM? CIME is not a model (it is a workflow). Does CESM use IPD? I don't know, but if it doesn't then it would either have to define the IPD or the GFS data types, same amount of work.

Dycore interoperability requires using either the IPD data types or the GFS data types (with my changes), but in dev/emc it also requires using CCPP. Given the amount of changes between the GFDL branch and dev/emc, how easy is it to sync code back and forth? @bensonr what is your take on this? Do we need some larger discussion on how to maintain compatibility between the different dycore branches?

If you think that retaining the "generality" of the IPD data types helps, then we can do one of the following, or something else if there is a better suggestion.

1. Keep a stripped-down version of the current IPD abstraction layer in fv3atm that simply passes the GFS types as IPD types to the dycore. You still have all of the other CCPP changes from this PR, though. And, by the way, the current code in dev/emc already uses the GFS data types (in tools/external_ic.F90), so I don't know how interoperable that part is. Maybe that file is just not (supposed to be) used outside the UFS.

2. Easier, but less elegant, in those few places where the IPD imports are replaced by GFS imports, have preprocessor directives that do something like the following code snippet. This is basically the same what the current IPD abstraction layer in fv3atm does.

#ifdef UFS
use GFS_typedefs, only : IPD_data => GFS_data
#else
use IPD_typedefs, only : IPD_data
#endif

Finally, note that @bensonr is ok with the changes proposed here. I am not married to them, but I am wondering about the overall strategy, and maybe this should have been discussed earlier on.

[DusanJovic-NOAA]

@climbfuji I would say remove as much code as possible while maintaining all the functionality we currently have. Less code, less bugs.

[climbfuji]

@junwang-noaa I came up with a compromise that minimizes the changes in this PR and allows for using either the GFS data types or the IPD data types in the dycore, depending on the presence of the GFS_TYPES preprocessor directive. With this, I can revert all the local changes from IPD_data to GFS_data etc. in driver/fvGFS/atmosphere.F90 and tools/fv_iau_mod.F90.

Please see 0e59620 and the additional preprocessor flag for the dycore in NOAA-EMC/fv3atm@52a4652. These two commits can be reverted or replaced if the suggested solution is not acceptable.

[junwang-noaa]

Dom, thanks to making the interfaces to dycore more general, this abstract layer (currently IPD data types) removes the dependence of the dycore code to GFS data and the dycore code can be still shared/extended to other host models.

…

On Wed, Jan 13, 2021 at 10:12 AM Dom Heinzeller ***@***.***> wrote: @junwang-noaa <https://github.com/junwang-noaa> I came up with a compromise that minimizes the changes in this PR and allows for using either the GFS data types or the IPD data types in the dycore, depending on the presence of the GFS_TYPES preprocessor directive. With this, I can revert all the local changes from IPD_data to GFS_data etc. in driver/fvGFS/atmosphere.F90 and tools/fv_iau_mod.F90. Please see 0e59620 <0e59620> and the additional preprocessor flag for the dycore in NOAA-EMC/fv3atm@ 52a4652 <NOAA-EMC/fv3atm@52a4652>. These two commits that can be reverted or replaced if the suggested solution is not acceptable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI7D6TOZA7IIAYX245RYBXDSZWZ55ANCNFSM4V2U5ZSQ> .

[climbfuji]

Dom, thanks to making the interfaces to dycore more general, this abstract layer (currently IPD data types) removes the dependence of the dycore code to GFS data and the dycore code can be still shared/extended to other host models.
…

Thanks, @junwang-noaa . I will wait for your fv3atm review before kicking off the regression tests.

[junwang-noaa]

CCPP version of fast physics becomes default in this PR, the GFDL version of fast process of GFDL MP fv3_cmp.F90 is removed. The future syncing dev/emc to GFDL dycore release has been discussed and recorded in issue #54. CCPP group may need to provide support in the future merge when there is fast physics updates.

[climbfuji]

@bensonr @XiaqiongZhou-NOAA This PR is ready, all regression tests passed against the existing baselines.

[climbfuji]

CCPP version of fast physics becomes default in this PR, the GFDL version of fast process of GFDL MP fv3_cmp.F90 is removed. The future syncing dev/emc to GFDL dycore release has been discussed and recorded in issue #54.

Thanks for the summary.

CCPP group may need to provide support in the future merge when there is fast physics updates.

And sure we will!

[XiaqiongZhou-NOAA]

Reviewed. It looks fine to me.