Fix real kind, alog10, rrtmgp dependences etc.

[dongli]

When incorporating CCPP in a host model, which does not use default real(8) compiler option, many errors pop up, so I added kind_phys explicitly in many places, and changed alog10 to log10, etc.

Also it seems RRTMGP added new codes, but meta files were not updated.

[gthompsnWRF]

I am not at all a fan of the incessant usage of kind_real into every variable. When that type is double precision, it causes a doubling of memory for a large number of variables. If a set of variables is internally fine to be single precision, no matter what compiler setting is used, then why can't the changes be minimized only to variables that need to pass through any interfaces? There are likely hundreds and hundreds of internal variables in this PR getting changed when perhaps only dozens need to be.

I find the code ugly to read with this addition and this seems to stem from improper explicit definitions of internal variables that someone decided double precision as the default for all physics helped. The diagnosis of which variables need to be 32 vs. 64 bits could have happened - I did it with my own physics scheme 15+ years ago.

Meanwhile, how has the code been working for this many years without these changes?

[dongli]

I agree that changing all real variables to real(kind_phys) increases memory usage. It is better that scheme developers could convert the type explicitly when calling subroutines or functions. It should not depend on specific compiler options like -real-size 8. I do not know how these codes could be compiled without any changes.

[dustinswales]

@dongli Thanks for opening this PR and cleaning up the working precision used within many ccpp schemes. Parameterizing the precision is something that we've been trying to encourage developers to do with new code, but we haven't gone back through all of the existing code.

@gthompsnWRF I agree that it is a bit verbose to have _kind_phys everywhere, but it avoids the portability issue @dongli is running into here. As for the memory issue, yeah double is more expensive, but there's nothing stopping the developer to parameterize their working precision with greater detail, using the kind_sngl_phys and kind_dbl_phys. This way single/double precision declarations/calculations can be done as needed.

[dustinswales]

Thanks for doing this, it's been on our "to-do list" for sometime now. The changes touch many schemes, so we will need to ensure that the POCs for the impacted code are aware, and all regression tests pass.
As for testing, have you done any testing within the UFS weather model? I expect there to be changes to the answers, just given the nature of the changes and the amount. (I can help run tests if need be)

[dustinswales]

This module doesn't use mo_rrtmgp_constants. Why add the dependency here?

[grantfirl]

Yes, thanks for chiming in @dustinswales . I didn't see the need for it directly, although I think it is a "second-order" dependency because mo_heating_rates uses the constants?

[dongli]

Sorry for the late reply! I worked out those dependencies one by one, and I haven't tested the changes in UFS weather model.

[dustinswales]

Interesting. Did you need these lines to build and run?

[dongli]

Yes. If it is not listed, the compilation failed.

[dustinswales]

I noticed that there is a local kp = kind_phys in this module. I suggest keeping only one working precision in the module. Also, if renaming, rename it when importing (e.g USE MACHINE , ONLY : kp => kind_phys)

[dustinswales]

Ideally the "double precision" should be "real(kind_dbl_phys)", from machine.F.

[dongli]

I cannot determine the precision of those variables or parameters one by one. It would be better that the scheme developers explicitly cast the type when it is needed.

[dustinswales]

@gthompsnWRF Regarding your note in the discussion about performance, would you say that everything in your scheme that is not explicitly defined as double-precision should be single-precision? (If so then I would suggest @dongli replace the changes here to use the kind_sngl_phys and kind_dbl_phys types, rather than kind_phys everywhere)

[grantfirl]

We need to use NCAR#918 as a guide. kind_phys needs to be kept for variables where single precision is acceptable, but can kind_phys can be set to either single or double precision depending on the user's needs. The issue is that someone actually needs to figure out which variables MUST stay in double precision rather than just adding kind_phys to everything, since that doesn't necessarily help anything (except perhaps with other hosts' compilation).

[grantfirl]

I agree with @gthompsnWRF that we would be better served to go through the code and figure out which variables need to stay double-precision to have "acceptable" results, whatever that is, rather than specifying every real variable by kind_phys automatically. I suppose this is preferable to having it unspecified and relying on compilation flags (which is probably the reason why the code can compile as-is -- current CCPP-compliant hosts just add the default real kind flag).

[gthompsnWRF]

I agree with @gthompsnWRF that we would be better served to go through the code and figure out which variables need to stay double-precision to have "acceptable" results, whatever that is, rather than specifying every real variable by kind_phys automatically. I suppose this is preferable to having it unspecified and relying on compilation flags (which is probably the reason why the code can compile as-is -- current CCPP-compliant hosts just add the default real kind flag).

Have I interpreted the conversion correctly that the proposed changes for making many real variables into kind_phys is now on hold? I think I mentioned already, but I painstakingly made only those variables that I know need double precision declared as such and positively nothing else since 32-bit precision suffices. Hence my objection to blanket changes that make the code so ugly when not needed. I agree there could be a handful of variables that may transfer to other places that might need casting to kind_phys. If that is absolutely necessary, then I do not object since I believe the modifications will be very few overall.

[grantfirl]

I agree with @gthompsnWRF that we would be better served to go through the code and figure out which variables need to stay double-precision to have "acceptable" results, whatever that is, rather than specifying every real variable by kind_phys automatically. I suppose this is preferable to having it unspecified and relying on compilation flags (which is probably the reason why the code can compile as-is -- current CCPP-compliant hosts just add the default real kind flag).

Have I interpreted the conversion correctly that the proposed changes for making many real variables into kind_phys is now on hold? I think I mentioned already, but I painstakingly made only those variables that I know need double precision declared as such and positively nothing else since 32-bit precision suffices. Hence my objection to blanket changes that make the code so ugly when not needed. I agree there could be a handful of variables that may transfer to other places that might need casting to kind_phys. If that is absolutely necessary, then I do not object since I believe the modifications will be very few overall.

I don't think that we've made a final decision on what to do with this PR. On one hand, it's a portability issue that the code as-is is forcing host models to use a default real kind flag to work with ambiguous reals. In this sense, adding kind_phys is a step in the right direction. If we look at how single- vs double-precision regression tests are implemented in the UFS (ufs-community/ufs-weather-model#1215), using kind_phys facilitates this. If we're running the CCPP in single precision, kind_phys reverts to single precision everywhere except where reals are explicitly kept as double precision using something other than kind_phys. Right now, in the UFS, by default, all physics are being run in double-precision unless explicitly changed to single-precision, so anywhere in your scheme where reals are ambiguous are being allocated as double-precision, bypassing your painstakingly chosen precision decisions (which, I think is to be commended, and much beyond what the vast majority of the rest of physics has done).

So, as I see it, the decision we face is: do we leave it as-is so that ambiguous reals rely on setting default real kind flags compiler flags in the host or do we take more control by using kind_phys everywhere so that it can easily be set/controlled outside of host compiler's flags? I think CCPP folks prefer the latter.

I think that we need to be a little more careful than what was done in this PR, personally, so, I'm leaning toward rejecting this as-is, but we do need to address the ambiguous reals, whether adding kind_phys is "ugly" or not -- we're modelers not car or clothes designers, right? There is also the issue of dependencies in the RRTMGP meta files that @dustinswales is looking into in this PR.

[dustinswales]

@gthompsnWRF As @grantfirl mentioned, it may not be aesthetically pleasing, but it makes the code base stronger and more portable. (To save space in your scheme, you could always do something like, use machine only: wp->kind_phys, dp->kind_dbl_phys, sp->kind_sngl_phys)

Even with the changes in this PR, my suggestion is to go a step further and parameterize all floating point precision, even instances where double precision is needed. We needed to do this in RRTMGP for some calculations.

[gthompsnWRF]

Clothing or car designer aside, I also cannot claim to be super knowledgeable about nuances with this idea of parameterizing all floating point precision, but I'm just not seeing the point to how the scheme is not already portable and strong regardless if the promoting everything to double precision happens or leaving it regular 32-bit.

If an entire bunch of internal variables declared REAL are perfectly safe being 32-bits in a series of internal calculations, then some compiler flag artificially causes them to become 64 bits, I'm thinking so be it. How does that produce non-portable, unsafe, or weak code?

The code has been running as-is in UFS for how many years? So what exact example of non-portable or weak code is there really? Is there burden of proof of innocence before guilt here? I'm truly trying to understand better, because it seems like over-engineering for a non-problem.

[dustinswales]

Clothing or car designer aside, I also cannot claim to be super knowledgeable about nuances with this idea of parameterizing all floating point precision, but I'm just not seeing the point to how the scheme is not already portable and strong regardless if the promoting everything to double precision happens or leaving it regular 32-bit.

If an entire bunch of internal variables declared REAL are perfectly safe being 32-bits in a series of internal calculations, then some compiler flag artificially causes them to become 64 bits, I'm thinking so be it. How does that produce non-portable, unsafe, or weak code?

The code has been running as-is in UFS for how many years? So what exact example of non-portable or weak code is there really? Is there burden of proof of innocence before guilt here? I'm truly trying to understand better, because it seems like over-engineering for a non-problem.

I hear ya, it's a bit verbose, but the case we have here is a perfect example of why this matters. The code broke because of type mismatches, which can be overlooked in UFS applications with the compilation flags they use (not to mention compiling this way wipes out all of the work you did to use single and double precision with intent internally within your scheme).

Ideally, all floating point declarations, and some operations, would be explicitly of _kind_phys, and if the scheme developer wants to go a step further they can define things of _kind_dbl_phys (OR _kind_sngl_phys) if they see fit. This seems like what you did, just not "parameterized". No?

[dongli]

Thanks for all your comments! I just try to use CCPP in my model so that it could be a test case for the more portability of CCPP. Sorry for mixing the dependencies of RRTMGP within this PR. It is OK that this PR is blocked since I am also not sure what is the best solution.

[dustinswales]

@dongli FYI. I ran the UFS Weather Model regression tests with these changes and the results differ. Since there are so many changes in this PR it may be hard to ascertain where the differences originate.

[grantfirl]

I think that the consensus of code managers is to systematically look at this issue and parameterize real kinds as necessary without affecting regression test baselines. This work should done be in conjunction with understanding which variables need to retain double-precision in every scheme.

An issue (#87) was opened and we're closing this PR.