ufs-community / ccpp-physics

UFS fork for CCPP
Other
4 stars 33 forks source link

Thompson-Eidhammer microphysics code formatting #147

Closed AndersJensen-NOAA closed 7 months ago

AndersJensen-NOAA commented 9 months ago

This PR is the first of many that will modernized, modularize, and streamline Thompson-Eidhammer microphysics.

This PR includes:

  1. The addition of parameterized kind: REAL -> real(kind_phys)
  2. Consistent indentation
  3. Removal of GOTO statements
AndersJensen-NOAA commented 9 months ago

@dustinswales This passes the rrfs regression test: /scratch2/BMC/wrfruc/jensen/dev/ufs-weather-model/tests/logs/RegressionTests_hera.log when RoverRv = 0.622. This will fail when RoverRv = R*oRv.

mzhangw commented 9 months ago

I am a little confused about the reconstruction of directories. Is this PR aimed at RRFS only? Do we have an agreement on an institutional directory name (GSL_*)?

yangfanglin commented 9 months ago

This is the first time I noticed a new GSL_cloud_phsyics directory has been created. This is concerning for me as we are moving towards community development and unified physics paradigm for all UFS applications. EMC, PSL, Greg Thompson and many others have all made significant contributions in the past few years to improve Thompson MP for RRFS, GFS and HAFS applications. We should continue the community development path.

lisa-bengtsson commented 9 months ago

This is the first time I noticed a new GSL_cloud_phsyics directory has been created. This is concerning for me as we are moving towards community development and unified physics paradigm for all UFS applications. EMC, PSL, Greg Thompson and many others have all made significant contributions in the past few years to improve Thompson MP for RRFS, GFS and HAFS applications. We should continue the community development path.

I have the same concern with the new gravity wave physics names, it doesn't seem on par with our pervious guidelines for community development.

ligiabernardet commented 9 months ago

Thank you @mzhangw for bringing this up. I second Lisa's and Fanglin's recommendation wrt not using "GSL" in the name/location of the scheme. We want to avoid organization names to stimulate community development.

dustinswales commented 9 months ago

@yangfanglin @mzhangw @lisa-bengtsson Sorry for the naming confusion. I opened an issue for more discussion, #151

grantfirl commented 9 months ago

@AndersJensen-NOAA @dustinswales I see that this PR started out as changes to the Thompson scheme, but now they are "hidden" by the move to the submodule. Could we split this up? That is, have one PR with the actual code changes that we can review/merge now, then another, separate PR for moving things around (preferably after the CCPP Physics Management meeting on 1/17)?

dustinswales commented 8 months ago

@AndersJensen-NOAA @dustinswales I see that this PR started out as changes to the Thompson scheme, but now they are "hidden" by the move to the submodule. Could we split this up? That is, have one PR with the actual code changes that we can review/merge now, then another, separate PR for moving things around (preferably after the CCPP Physics Management meeting on 1/17)?

@AndersJensen-NOAA I think it's best if we revert the move to a submodule for the time being. A larger discussion needs to be had, which will be soon. (My apologies for leading you down this rabbit hole). Can you revert to commit hash dd3040fa5ce72c34affd1fd18e1b6a7ae6236346

grantfirl commented 8 months ago

@dustinswales This passes the rrfs regression test: /scratch2/BMC/wrfruc/jensen/dev/ufs-weather-model/tests/logs/RegressionTests_hera.log when RoverRv = 0.622. This will fail when RoverRv = R*oRv.

I suggested passing in a bunch of physical constants that already exist from the host that will result in similar regression test failures. If we do one replacement, this is a good time to do them all, IMO. There is no point in calculating a new RoverRv when it already exists from the host with the CCPP standard name of ratio_of_dry_air_to_water_vapor_gas_constants.

grantfirl commented 8 months ago

This looks fine to me, @AndersJensen-NOAA. My only comments are related to using constants from the host so that this scheme is in line with all of the other CCPP schemes and the potentially superfluous double precision recasting. Since you've shown that your changes don't change the answer when you don't touch the constants, if you change one constant, I think that it makes sense to move to using whatever constants are available from the host so that we "rip the bandaid off" and get this scheme to be CCPP-compliant with respect to physical constants all at once. @dustinswales Objections?

grantfirl commented 8 months ago

@dustinswales I re-requested a review since you had requested changes. Your approval isnt required for merge, though.

AndersJensen-NOAA commented 8 months ago

@grantfirl Thanks for looking this over. I'll bring in constants and fix the double precision recasting.

grantfirl commented 8 months ago

@AndersJensen-NOAA Is this still being targeted for the RRFS release? Do you want some assistance with the physical constants changes? I'm super familiar with how to do this if so, and I don't mind making these changes and putting in a PR to your branch. Let me know.

AndersJensen-NOAA commented 7 months ago

@grantfirl, I would like this in the RRFS release, and yes, I would like assistance with constants changes. Feel free to add a PR. Thanks!

grantfirl commented 7 months ago

Closed in favor of https://github.com/ufs-community/ccpp-physics/pull/179 targeting the RRFS release branch.