Wrapper PR for assumed sizes, Fortran/metadata consistency fixes, bugfix Thompson MP, etc. #611
Conversation
update GFS_rrtmg_pre.F90 and GFS_rrtmg_pre.meta
not found errors found by capgen.
and out of order arguments for transition to capgen.
…cies, use assumed-size arrays
climbfuji
force-pushed
the
capgen_fixes_assumed_sizes
branch
2 times, most recently
from
6b81025
to
88dbbec
Compare
It is not a strict rule or requirement from CCPP side, but it makes sense. CISL has conducted performance tests and has found no difference between specifying dimensions (which are not known at compile time) and assumed sizes. The benefit using assumed sizes is that the compiler can check the bounds of the arrays, which it can't when you use explicit dimension. The only place where one has to be careful is that using (:) implies that the array starts at index 1. If it doesn't, one has to specify the lower bound (but can leave the upper bound open), see for example the snow layer arrays for NoahMP: |
There was a problem hiding this comment.
Conversion of all dummy arguments to assumed size arrays looks good. Since there are so many changes I guess we need to rely on compiler checks and regression tests to make sure everything is correct.
It would be nice to standardize all dummy arrays declarations. I see three styles:
real, dimension(:), intent :: var1, var2
real, intent, dimension(:) :: var1, var2
real, intent :: var1(:), var2(:)
Also spacing (:,:,:) vs. (:, :,:) should be consistent.
I agree. That's for another life I guess, will take forever. |
| its=its, ite=ite, jts=jts, jte=jte, kts=kts, kte=kte, & | ||
| errmsg=errmsg, errflg=errflg, reset=reset) | ||
|
|
||
| if (do_effective_radii) then |
There was a problem hiding this comment.
Isn't there a nicer way to do this?
In my PR#567, I had reduced this section to just a single call to mp_gt_driver from having two calls; now it has escalated to 4 calls. Is it really needed? Couldn't there be a simplification to call only one mp_gt_driver and auto-detect or switch some logical variable within to avoid what this combination is doing? It seems overly complex to me.
There was a problem hiding this comment.
The code crashes with Intel if I pass assumed-size arrays to mp_thompson_run. I think it is illegal to pass arrays that are non-existent further down. The definition of optional arrays is that you are not supposed to touch them if they are not present. It should not have worked in the past either, as far as I understand, but for some reason it did.
There was a problem hiding this comment.
One way around is to always pass the effective radii, i.e. remove the optional attribute in mp_thompson_run, and entirely rely on a flag whether to calculate them or not. Let me know if you prefer this, then we get back to two calls.
I could try something similar with the aerosols, too.
But it would require changes in mp_gt_driver - remove optional arguments and logic that tests for their presence.
There was a problem hiding this comment.
@gthompsnWRF This PR will go in today. I would like to revisit this question after my leave next week and come up with a solution that will reduce the number of "alternative" calls to mp_gt_driver to two or just one.
|
@climbfuji Dom, I agree with your approach In PR#567. I think these multiple calls are unnecessary. |
…pp-physics into capgen_fixes_assumed_sizes
…s/ccpp-physics into capgen_fixes_assumed_sizes
|
@junwang-noaa @DusanJovic-NOAA @SMoorthi-emc Thanks to the use of assumed-size arrays, I now have an answer (maybe the only one, maybe one of several) why the |
Done in ccpp-physics and ufs-weather-model. |
…pgen_fixes_assumed_sizes
…n CCPP (#284) This PR removes one entry from the ccpp_prebuild_config.py that is no longer required, and updates the submodule pointer for ccpp-physics for the changes described in NCAR/ccpp-physics#611. Also included: * workaround to propagate REPRO/Bitforbit mode correctly to CCPP - will be cleaned up in @DusanJovic-NOAA's next PR * tiny bugfix for aux3d arrays (active attribute in CCPP metadata was wrong), no impact on any of the tests, this is just a debugging feature Co-authored-by: Dustin Swales <[email protected]>
* MYNNsfc uniform real kind
This PR builds on #503 and #584. Including those, it contains the following changes:
intent(out)vaiablesatmosphere_heat_diffusivity/atmosphere_momentum_diffusivityfor all vertical levelsm_micro.F90, this became necessary when switching to assumed-size arraysFixes #357
Fixes #642
Associated PRs:
#611
NOAA-EMC/fv3atm#284
ufs-community/ufs-weather-model#527
For regression testing, see ufs-community/ufs-weather-model#527