NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 63 forks source link

Update ccpp_prebuild to support IAP model #452

Closed climbfuji closed 2 years ago

climbfuji commented 2 years ago

Description

Updates of ccpp-framework needed to support the IAP model (old version of CESM):

  1. Update of Var class in mkcap.py: To support passing arrays to the auto-generated group caps from the CCPP API whose starting indices are other than the default in Fortran (1), add logic to resolve dimension names in the local variable declarations (print_def_local) and in the declaration for interface variables (print_def_intent). For interface variables, an optional argument assumed_shape is used to leave the upper bound undefined, which allows the compiler to detect out of bound errors.
  2. Fix the logic that creates the interface variable declarations in mkstatic.py: in create_arguments_module_use_var_defs, iterate over the variables twice. In the first pass, process all scalars. In the second pass, process all arrays. This is so that any potential dimension that is used in the following array variable definitions is defined first to avoid violating the Fortran 2008 standard (https://community.intel.com/t5/Intel-Fortran-Compiler/Order-of-declaration-statements-with-and-without-implicit-typing/td-p/1176155).
  3. Further down, in the write function of the Group class in mkstatic.py, a similar trick is needed. Iterate over the piece of code that extracts all variables needed (including indices for components/slices of arrays and including their parents). We need to run this twice, because the dimensions of parent variables get added to the list of required variables (additional_variables_required) in the first pass. The logic in the script skips over variables in iteration 2 if they have already been added in iteration 1.
  4. A bit further down in the same function, simplify the logic that deals with adding variables to the argument list. This is possible because of the changes described in 3.

User interface changes?: No

Fixes #454

Testing: test removed: n/a unit tests: test [brew]> ./run_doctest.sh - all pass system tests: test [brew]> ./run_tests.sh - all pass manual testing: For regression testing with the UFS Weather Model, see https://github.com/ufs-community/ufs-weather-model/pull/1330

gold2718 commented 2 years ago

To me, it looks like this PR adds some new or different functionality to the framework. Is that correct?

climbfuji commented 2 years ago

To me, it looks like this PR adds some new or different functionality to the framework. Is that correct?

If you consider being able to pass blocked data structures (arrays) with a starting index other than 1 as new functionality, then yes. I believe capgen is perfectly capable of doing that already, but prebuild wasn't.

gold2718 commented 2 years ago

I believe capgen is perfectly capable of doing that already, but prebuild wasn't.

I am not sure that capgen will correctly support schemes with Fortran 77 arrays. This requirement has not been discussed by the CCPP governance committee.

climbfuji commented 2 years ago

I believe capgen is perfectly capable of doing that already, but prebuild wasn't.

I am not sure that capgen will correctly support schemes with Fortran 77 arrays. This requirement has not been discussed by the CCPP governance committee.

I don't understand. We are talking about being able to pass array(begchunk:endchunk) to the auto-generated APIs as an assumed-shape array: array(begchunk:) instead of passing it as array(:) because then Fortran thinks that the lower bound is one.

As far as I understand, capgen can do that already, because it supports horizontal_loop_extent_{begin,end}, while prebuild wasn't able to do that until now. It only understood 1:horizontal_loop_extent and 1:horizontal_dimension.

gold2718 commented 2 years ago

We are talking about being able to pass array(begchunk:endchunk) to the auto-generated APIs as an assumed-shape array: array(begchunk:) instead of passing it as array(:) because then Fortran thinks that the lower bound is one.

I am really confused. Except for pointers, it does not really matter how a subroutine is called. If the subroutine declares a dummy, non-pointer variable as real, intent(whatever) :: array(:), then Fortran will set the lower bound at 1. This is demonstrated by the simple program below:

module utils

   implicit none
   public :: test_lbound
contains
   subroutine test_lbound(array, callstr)
      real,             intent(in) :: array(:)
      character(len=*), intent(in) :: callstr

      write(6, '(2a,i0)') trim(callstr), ", LBOUND(array, 1) = ", LBOUND(array, 1)
   end subroutine test_lbound
end module utils

program test
   use utils, only: test_lbound
   implicit none

   real :: test_array(10:15)
   real :: test_array2(1:15)
   real, pointer :: test_array3(:)

   nullify(test_array3)

   call test_lbound(test_array, 'test_array')
   call test_lbound(test_array(13:15), 'test_array(13:15)')
   call test_lbound(test_array(:), 'test_array(:)')
   call test_lbound(test_array2, 'test_array2')
   call test_lbound(test_array2(3:5), 'test_array2(3:5)')
   call test_lbound(test_array2(:), 'test_array2(:)')
   call test_lbound(test_array3, 'test_array3')
   call test_lbound(test_array3(13:15), 'test_array3(13:15)')
   call test_lbound(test_array3(:), 'test_array3(:)')
end program test

If the subroutine declares the array with explicit bounds, those become the bounds. What am I missing?

climbfuji commented 2 years ago

The problem is demonstrated in this little program, I ran it with gfortran just now on my mac:

program test

    implicit none

    real, dimension(:,:), allocatable :: b

    allocate(b(1:7,-3:0))

    call mysub1(b)

    call mysub2(b,1,-3)

contains

    subroutine mysub1(a)
        real, intent(in) :: a(:,:)
        print '(a,2i6)', 'mysub1: Lower bounds of a:', lbound(a)
        print '(a,2i6)', 'mysub1: Upper bounds of a:', ubound(a)
    end subroutine mysub1

    subroutine mysub2(a,l1,l2)
        integer, intent(in) :: l1, l2
        real, intent(in) :: a(l1:,l2:)
        print '(a,2i6)', 'mysub2: Lower bounds of a:', lbound(a)
        print '(a,2i6)', 'mysub2: Upper bounds of a:', ubound(a)
    end subroutine mysub2

end program test

Output:

heinzell@JCSDA-L-18146:~/scratch/test_lbound_gfortran [brew-x86_64]> ./test.x
mysub1: Lower bounds of a:     1     1
mysub1: Upper bounds of a:     7     4
mysub2: Lower bounds of a:     1    -3
mysub2: Upper bounds of a:     7     0

This is exactly what happens in the auto-generated ccpp_prebuild suite/group caps, and this PR fixes it by passing and using the lower bound as in mysub2.

I believe capgen is perfectly capable of doing that already, but prebuild wasn't.

I am not sure that capgen will correctly support schemes with Fortran 77 arrays. This requirement has not been discussed by the CCPP governance committee.

I don't understand. We are talking about being able to pass array(begchunk:endchunk) to the auto-generated APIs as an assumed-shape array: array(begchunk:) instead of passing it as array(:) because then Fortran thinks that the lower bound is one.

As far as I understand, capgen can do that already, because it supports horizontal_loop_extent_{begin,end}, while prebuild wasn't able to do that until now. It only understood 1:horizontal_loop_extent and 1:horizontal_dimension.

gold2718 commented 2 years ago

This is exactly what happens in the auto-generated ccpp_prebuild suite/group caps, and this PR fixes it by passing and using the lower bound as in mysub2

This is all very straightforward and expected. My question is why does it matter? In your example program, what code acts as the CCPP-generated cap?

climbfuji commented 2 years ago

msub1 (previously) and mysub2 (this PR)

On Jul 17, 2022, at 10:35 PM, goldy @.***> wrote:

This is exactly what happens in the auto-generated ccpp_prebuild suite/group caps, and this PR fixes it by passing and using the lower bound as in mysub2

This is all very straightforward and expected. My question is why does it matter? In your example program, what code acts as the CCPP-generated cap?

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-framework/pull/452#issuecomment-1186702241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5C2RISXL6NHFHYVFD6TN3VUS7H3ANCNFSM5WN7NT4A. You are receiving this because you authored the thread.

gold2718 commented 2 years ago

So my contention is that it does not matter what happens in the cap, what matters is what happens in the schemes. Here, I have renamed mysub1 to cap_pre and mysub2 to cap_post and introduced two schemes. I see no difference in the schemes from being called with different caps.

program test

   implicit none

   real, dimension(:,:), allocatable :: b

   allocate(b(1:7,-3:0))

   call cap_pre(b,1,-3)

   call cap_post(b,1,-3)

contains

   subroutine cap_pre(a, l1, l2)
      real,    intent(in) :: a(:,:)
      integer, intent(in) :: l1
      integer, intent(in) :: l2
      print '(a,2i6)', 'cap_pre: Lower bounds of a:', lbound(a)
      print '(a,2i6)', 'cap_pre: Upper bounds of a:', ubound(a)
      call scheme_a(a)
      call scheme_b(a, l1, l2)
   end subroutine cap_pre

   subroutine cap_post(a,l1,l2)
      integer, intent(in) :: l1, l2
      real, intent(in) :: a(l1:,l2:)
      print '(a,2i6)', 'cap_post: Lower bounds of a:', lbound(a)
      print '(a,2i6)', 'cap_post: Upper bounds of a:', ubound(a)
      call scheme_a(a(l1:,l2:))
      call scheme_b(a(l1:,l2:), l1, l2)
   end subroutine cap_post

   subroutine scheme_a(a)
      real, intent(in) :: a(:,:)
      print '(a,2i6)', 'scheme_a: Lower bounds of a:', lbound(a)
      print '(a,2i6)', 'scheme_a: Upper bounds of a:', ubound(a)
   end subroutine scheme_a

   subroutine scheme_b(a,l1,l2)
      integer, intent(in) :: l1, l2
      real, intent(in) :: a(l1:,l2:)
      print '(a,2i6)', 'scheme_b: Lower bounds of a:', lbound(a)
      print '(a,2i6)', 'scheme_b: Upper bounds of a:', ubound(a)
   end subroutine scheme_b

end program test

So I still do not see why it matters what the cap does. What am I missing?

climbfuji commented 2 years ago

So my contention is that it does not matter what happens in the cap, what matters is what happens in the schemes. Here, I have renamed mysub1 to cap_pre and mysub2 to cap_post and introduced two schemes. I see no difference in the schemes from being called with different caps.

program test

   implicit none

   real, dimension(:,:), allocatable :: b

   allocate(b(1:7,-3:0))

   call cap_pre(b,1,-3)

   call cap_post(b,1,-3)

contains

   subroutine cap_pre(a, l1, l2)
      real,    intent(in) :: a(:,:)
      integer, intent(in) :: l1
      integer, intent(in) :: l2
      print '(a,2i6)', 'cap_pre: Lower bounds of a:', lbound(a)
      print '(a,2i6)', 'cap_pre: Upper bounds of a:', ubound(a)
      call scheme_a(a)
      call scheme_b(a, l1, l2)
   end subroutine cap_pre

   subroutine cap_post(a,l1,l2)
      integer, intent(in) :: l1, l2
      real, intent(in) :: a(l1:,l2:)
      print '(a,2i6)', 'cap_post: Lower bounds of a:', lbound(a)
      print '(a,2i6)', 'cap_post: Upper bounds of a:', ubound(a)
      call scheme_a(a(l1:,l2:))
      call scheme_b(a(l1:,l2:), l1, l2)
   end subroutine cap_post

   subroutine scheme_a(a)
      real, intent(in) :: a(:,:)
      print '(a,2i6)', 'scheme_a: Lower bounds of a:', lbound(a)
      print '(a,2i6)', 'scheme_a: Upper bounds of a:', ubound(a)
   end subroutine scheme_a

   subroutine scheme_b(a,l1,l2)
      integer, intent(in) :: l1, l2
      real, intent(in) :: a(l1:,l2:)
      print '(a,2i6)', 'scheme_b: Lower bounds of a:', lbound(a)
      print '(a,2i6)', 'scheme_b: Upper bounds of a:', ubound(a)
   end subroutine scheme_b

end program test

So I still do not see why it matters what the cap does. What am I missing?

It does matter in the caps (at least for ccpp_prebuild.py) when variable manipulations/testing comes into play. For instance, automatic de-blocking of blocked data structures relies on the correct indices being accessed (at least the way it is written now, but possibly in general, too). And there are also automatic debugging features in prebuild (activated with --debug) that test for the allocation of the array to match the dimensions in the metadata.

I agree that schemes shouldn't depend on these indices when it comes to horizontal dimensions etc. Some (e.g. NOAH LSM) do depend on the soil vertical levels running from -3 to 0, but that's ok works fine unless the cap introduces variable manipulations.

Therefore it is safest to ensure that the array bounds are known everywhere in the code, including the auto-generated caps. This may only matter for ccpp_prebuild.py, but the changes introduced in this PR don't affect what capgen.py is doing or has to do. If capgen.py manipulates variables in the auto-generated caps differently and doesn't need the correct lower and upper bounds, then that's totally fine.

gold2718 commented 2 years ago

Thank you for that work I am still unclear on how much of this would ever really be needed for capgen. However, it seems to be introducing a new requirement for the framework which should at least be discussed and agreed on by the governance committee. I think the next step is to have that discussion and get the framework requirements explicitly spelled out and agreed upon.

climbfuji commented 2 years ago

Sounds good to me. I don't think anything is or will be needed on the capgen side, but we can talk about that when we meet next time (not tomorrow - at the UFS workshop in College Park this week).

grantfirl commented 2 years ago

@gold2718 @climbfuji Circling back to this PR, the UFS code managers have this in the merge queue for next Wednesday (Aug 3). Please let me know if we should communicate a delay in this timeline. I understand that Steve would like to discuss this, but I don't know when that discussion will take place...

gold2718 commented 2 years ago

@grantfirl, I think this can be discussed on Tuesday. Are there changes in this PR needed by the UFS?

climbfuji commented 2 years ago

None of the changes are needed for the UFS, it's more that there is a good slot available where we can merge a few b4b identical PRs for ccpp-physics and framework at the same time.

climbfuji commented 2 years ago

Thank you, @gold2718 for reviewing the PR. Will try to put more information in the issue than there is already next time, and create the issue as early as possible.

@grantfirl @mkavulich This PR needs your reviews, too. Thanks ...

BrianCurtis-NOAA commented 2 years ago

We are ready for merge on the UFSWM side.

climbfuji commented 2 years ago

Thanks, Brian. Merged - new hash is 4daaa24.