search

NCAR / ccpp-physics

GFS physics for CCPP
Other
58 stars 145 forks source link

One-to-one schemes/files #896

Closed grantfirl closed 2 years ago

grantfirl commented 2 years ago

This is an organizational change only, whereby:

  1. files that contained multiple CCPP entry-point schemes/modules (mostly interstitials) are split such that every CCPP entry-point scheme has its own file. This change may aid clarity for debugging and file purpose.
  2. scheme/module names should match filenames (also for clarity)

NOTE 1: To minimize upstream PR changes, source file names are changed to match scheme/module names.

NOTE 2: GFS_debug.F90 contains 11 schemes/modules, but I chose to leave that as-is because it is not an official part of any suite and 10 additional files would add to directory clutter even more.

grantfirl commented 2 years ago

Fixes https://github.com/NCAR/ccpp-physics/issues/878

climbfuji commented 2 years ago

Quick question: What about GFS_debug.F90?

grantfirl commented 2 years ago

Quick question: What about GFS_debug.F90?

This was the odd man out. I did not break this up because there were so many schemes and it isn't part of an "official" suite. I can split it if necessary, though.

climbfuji commented 2 years ago

Quick question: What about GFS_debug.F90?

This was the odd man out. I did not break this up because there were so many schemes and it isn't part of an "official" suite. I can split it if necessary, though.

I was just curious. Let's see what the others say.

dustinswales commented 2 years ago

@grantfirl Just for my own understanding... RRTMGP has _init and _runs routines in some scheme files. But I believe _init and _run routines are treated differently when generating the caps?

grantfirl commented 2 years ago

@grantfirl Just for my own understanding... RRTMGP has _init and _runs routines in some scheme files. But I believe _init and _run routines are treated differently when generating the caps?

@dustinswales It was my understanding that his PR is meant to 1) make sure that each CCPP scheme has its own file and 2) the name of the file should match the name of the module/scheme. All phases of each scheme (init, timestep_init, run...) should stay within its module/file. Since the RRTMGP parameterization contains many CCPP "schemes", this PR shouldn't touch any of those files since as long as filename = scheme module name AND each file contains one module (with all phases), then that is the desired organization. The question of whether to combine some of the RRTMGP-related "schemes" is separate from this PR and its organization is mostly up to the developers (you and Robert).

dustinswales commented 2 years ago

@grantfirl Thanks for the explanation. FYI, I have plans to reduce the number of GP scheme files. I feel that there may be a tad too many...

ChunxiZhang-NOAA commented 2 years ago

If I understand correctly, the interstitial code can be significantly reduced if each scheme put it's pre- and post- work inside timestep_init and timestep_finalize, respectively.

grantfirl commented 2 years ago

If I understand correctly, the interstitial code can be significantly reduced if each scheme put it's pre- and post- work inside timestep_init and timestep_finalize, respectively.

@ChunxiZhang-NOAA I think that there may be instances where this is strictly true, although I'm not sure that it is the best path for two reasons:

  1. Many pre/post interstitials rely on being called at specific places in the suite order during the run phase.
  2. One reason why interstitial schemes exist is for (at least potential) portability. For example, many pre/post schemes prepare data or calculate diagnostics that make sense in the context of a GFS-based suite, but may not make sense in the context of a different host's suite. The primary schemes are supposed to only contain code that is needed to parameterize some physical process such that they can be used outside of the UFS as well. The extent to which this happens right now may be minimal, although one of the goals of the CCPP is to facilitate this.

All of this is to say that if the only goal was to simplify organization of the code, there are many places to combine things as you suggested, although that isn't the only goal of the CCPP.

barlage commented 2 years ago

@grantfirl @cenlinhe @HelinWei-NOAA

I like the organization of lsm_ files. Any reason to not have lsm_noahmp to follow that same convention?

grantfirl commented 2 years ago

@grantfirl @cenlinhe @HelinWei-NOAA

I like the organization of lsm_ files. Any reason to not have lsm_noahmp to follow that same convention?

That's a good point @barlage. I edited the PR description to explain the renaming process a bit more, but in general, in order to avoid having to change a bunch of suite definition files, when there was a clash between the scheme/module name and the filename, I changed the filename to match the scheme/module name. The scheme/module name for NOAHMP was noahmpdrv, so the filename was changed to that. If we want to change the scheme/module name, I'm thinking that they may belong in a different PR since this is already modifying 100+ files. Thoughts @ligiabernardet @climbfuji ?

grantfirl commented 2 years ago

@ligiabernardet @climbfuji This reorganization PR is changing the answer for suites using CS convection for some reason. Note that the debug tests ARE b4b, so I'm guessing that it is an optimization issue with that scheme. Should we just continue with this PR with the changed results?

grantfirl commented 2 years ago

Update on the failed tests. Jun asked to test the develop branch in REPRO mode against these changes in REPRO mode, and the previously-failing tests PASSED in REPRO mode. This should be acceptable, IMO.

yangfanglin commented 2 years ago

@SMoorthi-emc Moorthi, would you like to comment on "This reorganization PR is changing the answer for suites using CS convection" ? Tests in REPRO mode passed.

SMoorthi-emc commented 2 years ago

Fanglin, I am not sure what this reorganization is. I can't comment as I have no idea what is being done. Moorthi

On Fri, Apr 15, 2022 at 12:01 PM Fanglin Yang @.***> wrote:

@SMoorthi-emc https://github.com/SMoorthi-emc Moorthi, would you like to comment on "This reorganization PR is changing the answer for suites using CS convection" ? Tests in REPRO mode passed.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/896#issuecomment-1100196853, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALLVRYTV5RKA5TJA5B5S5ILVFGHELANCNFSM5TLA2JZA . You are receiving this because you were mentioned.Message ID: @.***>

-- Dr. Shrinivas Moorthi Research Meteorologist Modeling and Data Assimilation Branch Environmental Modeling Center / National Centers for Environmental Prediction 5830 University Research Court - (W/NP23), College Park MD 20740 USA Tel: (301)683-3718

e-mail: @.*** Phone: (301) 683-3718 Fax: (301) 683-3718

grantfirl commented 2 years ago

@SMoorthi-emc As the PR description says, I was asked to make sure that each CCPP "scheme" is in its own file and that the scheme name matches the filename. The relevant change for CS convection was to put the cs_conv_pre and cs_conv_post in their own files instead of in cs_conv.F90. Due to compiler optimization, this apparently changes results in PROD mode only (results are identical in REPRO and DEBUG modes). All other changes in this PR do not change the answer in any mode.

AnningCheng-NOAA commented 2 years ago

I have just taken some time to follow the thread. I think the reorganization makes the code easy to follow, but the results should not change even in PROD mode because the modification is on the software side. Am I missing something?

On Fri, Apr 15, 2022 at 12:44 PM Grant Firl @.***> wrote:

@SMoorthi-emc https://github.com/SMoorthi-emc As the PR description says, I was asked to make sure that each CCPP "scheme" is in its own file and that the scheme name matches the filename. The relevant change for CS convection was to put the cs_conv_pre and cs_conv_post in their own files instead of in cs_conv.F90. Due to compiler optimization, this apparently changes results in PROD mode only (results are identical in REPRO and DEBUG modes). All other changes in this PR do not change the answer in any mode.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/896#issuecomment-1100220354, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALQPMIJPXWAOODTMNFXWOWLVFGMGNANCNFSM5TLA2JZA . You are receiving this because your review was requested.Message ID: @.***>

grantfirl commented 2 years ago

I have just taken some time to follow the thread. I think the reorganization makes the code easy to follow, but the results should not change even in PROD mode because the modification is on the software side. Am I missing something? … On Fri, Apr 15, 2022 at 12:44 PM Grant Firl @.> wrote: @SMoorthi-emc https://github.com/SMoorthi-emc As the PR description says, I was asked to make sure that each CCPP "scheme" is in its own file and that the scheme name matches the filename. The relevant change for CS convection was to put the cs_conv_pre and cs_conv_post in their own files instead of in cs_conv.F90. Due to compiler optimization, this apparently changes results in PROD mode only (results are identical in REPRO and DEBUG modes). All other changes in this PR do not change the answer in any mode. — Reply to this email directly, view it on GitHub <#896 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALQPMIJPXWAOODTMNFXWOWLVFGMGNANCNFSM5TLA2JZA . You are receiving this because your review was requested.Message ID: @.>

@AnningCheng-NOAA With aggressive optimization as with PROD mode, simple code changes can often lead to B4B differences, e.g. putting a write statement in an "unlucky" place. People more knowledgeable about this topic than me (@climbfuji) have often made the point that lower optimization (as in REPRO or DEBUG mode) should be used to detect whether code changes that shouldn't change the answer are "correct". This is the case for all changes in this PR.

ChunxiZhang-NOAA commented 2 years ago

@SMoorthi-emc Not sure if this is related to the failure of PROD mode. The dimension for variable fscav and fswtr is number_of_tracers_scavenged, but in subroutine CS_CUMLUS, you declared it with number_of_tracers_for_CS, see local name ntr. If number_of_tracers_for_CS=number_of_tracers_scavenged, it will be ok.

AnningCheng-NOAA commented 2 years ago

They should be the same thing. I have not seen any difference in the code so far, but had better leave the answer to Moorthi.

On Fri, Apr 15, 2022 at 3:14 PM ChunxiZhang-NOAA @.***> wrote:

@SMoorthi-emc https://github.com/SMoorthi-emc Not sure if this is related to the failure of PROD mode. The dimension for variable fscav and fswtr is number_of_tracers_scavenged, but in subroutine CS_CUMLUS, you declared it with number_of_tracers_for_CS, see local name ntr. If number_of_tracers_for_CS=number_of_tracers_scavenged, it will be ok.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/896#issuecomment-1100301957, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALQPMII4GJZHPAGLQLBQ5KDVFG5ZTANCNFSM5TLA2JZA . You are receiving this because you were mentioned.Message ID: @.***>

grantfirl commented 2 years ago

Please modify the file name following the "!>\file" in files renamed without changes. I also left some minor Doxygen format suggestions

Thanks for the review @mzhangw and for catching the Doxygen mistakes. I believe that I've addressed your comments if you have time for a quick re-review. The change in filenames might also mean changes for the Doxygen configuration file. Is ccpp/physics/physics/docs/ccpp_doxyfile still used for this?

mzhangw commented 2 years ago

Please modify the file name following the "!>\file" in files renamed without changes. I also left some minor Doxygen format suggestions

Thanks for the review @mzhangw and for catching the Doxygen mistakes. I believe that I've addressed your comments if you have time for a quick re-review. The change in filenames might also mean changes for the Doxygen configuration file. Is ccpp/physics/physics/docs/ccpp_doxyfile still used for this?

mzhangw commented 2 years ago

Is it aimed for v6? If so, I will add new files to v6 doxyfile in my SciDoc branch when it is merged.

climbfuji commented 2 years ago

Yes, it is for v6.

mzhangw commented 2 years ago

Could you mark this PR with the CCPPv6 label?

mzhangw commented 2 years ago

This PR also deletes all empty _init and _finalize subroutines. Shall we go further to do the same to other codes, e.g. GFS_rrtmg_pre.F90

grantfirl commented 2 years ago

This PR also deletes all empty _init and _finalize subroutines. Shall we go further to do the same to other codes, e.g. GFS_rrtmg_pre.F90

That is something that I did for files where I split out modules for cleanup. Since the empty subroutines are not required anymore, it was very easy to do that. IMO, I don't think this is a high priority to do the rest since they don't break anything. A higher priority in my mind is to get rid of importing constants from physcons, but this can sometimes lead to b4b differences, so this should probably be its own task.

mzhangw commented 2 years ago

This PR also deletes all empty _init and _finalize subroutines. Shall we go further to do the same to other codes, e.g. GFS_rrtmg_pre.F90

That is something that I did for files where I split out modules for cleanup. Since the empty subroutines are not required anymore, it was very easy to do that. IMO, I don't think this is a high priority to do the rest since they don't break anything. A higher priority in my mind is to get rid of importing constants from physcons, but this can sometimes lead to b4b differences, so this should probably be its own task.

OK, the latest practice is in conflict with techdoc section 2.1, which need to be updated in v6 version.

grantfirl commented 2 years ago

This PR also deletes all empty _init and _finalize subroutines. Shall we go further to do the same to other codes, e.g. GFS_rrtmg_pre.F90

That is something that I did for files where I split out modules for cleanup. Since the empty subroutines are not required anymore, it was very easy to do that. IMO, I don't think this is a high priority to do the rest since they don't break anything. A higher priority in my mind is to get rid of importing constants from physcons, but this can sometimes lead to b4b differences, so this should probably be its own task.

OK, the latest practice is in conflict with techdoc section 2.1, which need to be updated in v6 version.

Yes, I agree that the docs should be updated to reflect that subroutines for non-used phases are not needed anymore. Also, the timestep_init and timestep_finalize phases aren't mentioned at all.

SMoorthi-emc commented 1 year ago

Chunxi,

On Fri, Apr 15, 2022 at 3:14 PM ChunxiZhang-NOAA @.***> wrote:

@SMoorthi-emc https://github.com/SMoorthi-emc Not sure if this is related to the failure of PROD mode. The dimension for variable fscav and fswtr is number_of_tracers_scavenged, but in subroutine CS_CUMLUS, you declared it with number_of_tracers_for_CS, see local name ntr. If number_of_tracers_for_CS=number_of_tracers_scavenged, it will be ok.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/896#issuecomment-1100301957, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALLVRYW6NAQQYAWGD4UR6Y3VFG5ZRANCNFSM5TLA2JZA . You are receiving this because you were mentioned.Message ID: @.***>

-- Dr. Shrinivas Moorthi Research Meteorologist Modeling and Data Assimilation Branch Environmental Modeling Center / National Centers for Environmental Prediction 5830 University Research Court - (W/NP23), College Park MD 20740 USA Tel: (301)683-3718

e-mail: @.*** Phone: (301) 683-3718 Fax: (301) 683-3718