Thompson-Eidhammer microphysics code formatting #147

AndersJensen-NOAA · 2023-12-19T02:54:36Z

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

This PR includes:

The addition of parameterized kind: REAL -> real(kind_phys)
Consistent indentation
Removal of GOTO statements

dustinswales

@AndersJensen-NOAA Good job with this herculean effort!
I still think there is some work needed to clean up parameterizing the precision. For example, within the scheme there are still many instance of inline conversions that aren't using the parameterized precision (e.g. DBLE(var) instead of REAL(var,kind=kind_dbl_prec)).

I know this makes some things a bit long winded, so for brevity, you could redirect the precisions to something shorter. Something like
From:
use machine, only: kind_phys, kind_sngl_prec, kind_dbl_prec
To:
use machine, only: wp => kind_phys, sp => kind_sngl_prec, dp => kind_dbl_prec
Then instances of DBLE(var) could be changed to, the slightly shorter, real(var, kind=dp)

physics/module_mp_thompson.F90

AndersJensen-NOAA · 2023-12-22T17:48:12Z

@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 · 2024-01-03T16:37:02Z

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 · 2024-01-03T17:18:43Z

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 · 2024-01-03T17:20:58Z

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 · 2024-01-03T17:45:09Z

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 · 2024-01-03T18:36:18Z

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

grantfirl · 2024-01-10T18:27:25Z

physics/module_mp_radar.F90

@@ -1,625 +0,0 @@
-!>\file module_mp_radar.F90


GFDL MP also uses this module. Does this really belong in the submodule repo if other MP schemes depend on it?

grantfirl · 2024-01-10T18:29:32Z

physics/module_mp_thompson.F90

@@ -1,6545 +0,0 @@
-!>\file module_mp_thompson.F90


Were any changes made to this file when it was moved? Since there isn't an associated PR for the new submodule, it's impossible to tell/review any changes that you may have put in the moved file.

grantfirl · 2024-01-10T18:44:15Z

@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 · 2024-01-11T16:28:56Z

@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 dd3040f

… into thompson_refactor

grantfirl · 2024-01-23T21:55:50Z

physics/MP/Thompson/module_mp_thompson.F90

+         xDx(nbi+1) = D0s*2.0_dp
+         do n = 2, nbi
+            xDx(n) = exp(real(n-1, kind=dp)/real(nbi, kind=dp) &
+                     *log(real(xDx(nbi+1)/xDx(1), kind=dp)) + log(real(xDx(1), kind=dp)))


Is it really required to have real(X,dp) inside the logs when the argument is already the precision required? This same question obviously applies to many of the statements below too.

I see @dustinswales comment above, but I think it is referring to the recasting of the integers, right? I don't see why we need to recast xDx when it is already double precision.

@grantfirl Correct, integers only. xD is defined type dp already

@grantfirl I fixed as many of these as I could find. See latest commit.

grantfirl · 2024-01-23T21:59:08Z

physics/MP/Thompson/module_mp_thompson.F90

+                  return
+               else
+                  write(*,'(a)') 'Logic error in mp_gt_driver: provide either tt or th+pii'
+                  stop


Here is a stop statement to clean up in the future. CCPP schemes shouldn't use STOP and should do what is done above with errmsg, errflg, return.

OK, nevermind, I see that this should never execute in the CCPP.

grantfirl · 2024-01-23T22:05:45Z

physics/MP/Thompson/module_mp_thompson.F90

-      DOUBLE PRECISION, PARAMETER, PRIVATE:: N_avo = 6.022E23            !< Avogadro number [1/mol]
-      DOUBLE PRECISION, PARAMETER, PRIVATE:: ma_w = M_w / N_avo          !< mass of water molecule [kg]
-      REAL, PARAMETER, PRIVATE:: ar_volume = 4./3.*PI*(2.5e-6)**3        !< assume radius of 0.025 micrometer, 2.5e-6 cm
+   real(wp), parameter, private :: Rv = 461.5


All of these should be coming in through the argument list (lines 194-212). Otherwise, this may be introducing inconsistencies between this scheme and the rest of physics and the host model.

@grantfirl Let's save this for another PR.

grantfirl · 2024-01-23T22:22:49Z

@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 · 2024-01-23T22:27:35Z

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 · 2024-01-24T14:54:18Z

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

AndersJensen-NOAA · 2024-01-25T23:58:20Z

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

grantfirl · 2024-02-09T15:51:41Z

@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 · 2024-02-12T20:22:43Z

@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 · 2024-02-28T19:41:48Z

Closed in favor of #179 targeting the RRFS release branch.

AndersJensen-NOAA and others added 4 commits December 7, 2023 14:01

Thompson refactor

3b41b8a

Missing intentation

a3e0d45

Final formatted and CCN table sngl_prec

10a17a9

Merge branch 'ufs-community:ufs/dev' into thompson_refactor

e2d6c1e

AndersJensen-NOAA requested review from gthompsnWRF, RuiyuSun, grantfirl, Qingfu-Liu and dustinswales as code owners December 19, 2023 02:54

dustinswales requested changes Dec 19, 2023

View reviewed changes

AndersJensen-NOAA added 2 commits December 21, 2023 11:56

Changes from review 1

72370a4

Shorten kind type notation

dd3040f

AndersJensen-NOAA requested a review from mzhangw as a code owner January 3, 2024 04:24

grantfirl reviewed Jan 10, 2024

View reviewed changes

grantfirl force-pushed the thompson_refactor branch from b139e51 to dd3040f Compare January 16, 2024 16:06

Merge branch 'ufs/dev' of https://github.com/ufs-community/ccpp-physics…

dcf6422

… into thompson_refactor

grantfirl reviewed Jan 23, 2024

View reviewed changes

grantfirl requested a review from dustinswales January 24, 2024 14:53

Qingfu-Liu approved these changes Jan 25, 2024

View reviewed changes

Fixes to precision

f2ea60d

grantfirl mentioned this pull request Jan 31, 2024

UFS-dev PR#118 NCAR/ccpp-physics#1040

Merged

This was referenced Feb 27, 2024

[production/RRFS.v1] physics updates for RRFS.v1 code freeze #176

Merged

[production/RRFS.v1] Thompson-Eidhammer microphysics code formatting #179

Merged

grantfirl closed this Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thompson-Eidhammer microphysics code formatting #147

Thompson-Eidhammer microphysics code formatting #147

AndersJensen-NOAA commented Dec 19, 2023

dustinswales left a comment •

edited

Loading

AndersJensen-NOAA commented Dec 22, 2023

mzhangw commented Jan 3, 2024

yangfanglin commented Jan 3, 2024 •

edited

Loading

lisa-bengtsson commented Jan 3, 2024

ligiabernardet commented Jan 3, 2024

dustinswales commented Jan 3, 2024

grantfirl Jan 10, 2024

grantfirl Jan 10, 2024

grantfirl commented Jan 10, 2024

dustinswales commented Jan 11, 2024

grantfirl Jan 23, 2024

grantfirl Jan 23, 2024

dustinswales Jan 24, 2024

AndersJensen-NOAA Jan 26, 2024

grantfirl Jan 23, 2024

grantfirl Jan 23, 2024

grantfirl Jan 23, 2024

AndersJensen-NOAA Jan 26, 2024

grantfirl commented Jan 23, 2024

grantfirl commented Jan 23, 2024

grantfirl commented Jan 24, 2024

AndersJensen-NOAA commented Jan 25, 2024

grantfirl commented Feb 9, 2024

AndersJensen-NOAA commented Feb 12, 2024

grantfirl commented Feb 28, 2024

Thompson-Eidhammer microphysics code formatting #147

Thompson-Eidhammer microphysics code formatting #147

Conversation

AndersJensen-NOAA commented Dec 19, 2023

dustinswales left a comment • edited Loading

Choose a reason for hiding this comment

AndersJensen-NOAA commented Dec 22, 2023

mzhangw commented Jan 3, 2024

yangfanglin commented Jan 3, 2024 • edited Loading

lisa-bengtsson commented Jan 3, 2024

ligiabernardet commented Jan 3, 2024

dustinswales commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantfirl commented Jan 10, 2024

dustinswales commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantfirl commented Jan 23, 2024

grantfirl commented Jan 23, 2024

grantfirl commented Jan 24, 2024

AndersJensen-NOAA commented Jan 25, 2024

grantfirl commented Feb 9, 2024

AndersJensen-NOAA commented Feb 12, 2024

grantfirl commented Feb 28, 2024

dustinswales left a comment •

edited

Loading

yangfanglin commented Jan 3, 2024 •

edited

Loading