Clean up use and implementation of accumulMod

[billsacks]

Bill Sacks < sacks@ucar.edu > - 2014-08-05 15:32:30 -0600 Bugzilla Id: 2023 Bugzilla CC: erik@ucar.edu, muszala@ucar.edu, mvertens@ucar.edu, rfisher@ucar.edu,

Currently, on Mariana's refactor branch, there is a lot of verbosity and repeated boilerplate code needed in the InitAccVars routines, which initialize variables associated with time-accumulated fields. For example:

    begp = bounds%begp; endp = bounds%endp

    ! Allocate needed dynamic memory for single level pft field
    allocate(rbufslp(begp:endp), stat=ier)
    if (ier/=0) then
       write(iulog,*)' in '
       call endrun(msg=" allocation error for rbufslp"//&
            errMsg(__FILE__, __LINE__))
    endif

    nstep = get_nstep()

    call extract_accum_field ('AGDDTW', rbufslp, nstep) 
    this%agddtw_patch(begp:endp) = rbufslp(begp:endp)

    call extract_accum_field ('AGDD', rbufslp, nstep) 
    this%agdd_patch(begp:endp) = rbufslp(begp:endp)

    deallocate(rbufslp)

Could this be replaced by simply the following?:

    call extract_accum_field('AGDDTW', this%agddtw_patch(bounds%begp:bounds%endp))

    call extract_accum_field('AGDD', this%agdd_patch(begp:endp))

The two major changes here are:

(1) the extract_accum_field routine gets nstep, rather than requiring the caller to obtain and pass in this value.

(2) I avoid the use of rbufslp - which may be needed in some cases, but doesn't appear to be needed here.

Edit (2019-11-15): It looks like rbufslp and similar variables are used in order to pass in pointers with particular lower bounds. I think this isn't needed with the current setup of having these routines always called outside a clump loop. But even if this is needed (e.g., to allow calling these routines from inside a clump loop), I think we could avoid this with a mechanism like what's suggested in https://github.com/ESCOMP/CTSM/issues/30#issuecomment-554031145.

[billsacks]

Bill Sacks < sacks@ucar.edu > - 2016-08-29 14:40:44 -0600

We should also consider storing individual objects for each accumul object, rather than the current storage in one centralized location with fields accessed by string. This would be similar to what I'm doing for the annual_flux_dribbler_type. The one downside of this is that clients would need to explicitly call Restart for each field, but the feeling at today's clm-cmt meeting is that that's worth it.

[billsacks]

Bill Sacks < sacks@ucar.edu > - 2016-08-29 14:42:09 -0600

Note that accumulMod routines currently can't be called from inside a clump (threading) loop. I believe that it would be very difficult to remove that limitation until we rework threading to be done at a higher level. This is because the accumulMod infrastructure is general enough to handle fields at any level (gridcell, column, patch, etc.), and if you want to have code inside a clump loop, you need to declare the lower bounds of any array arguments... these two things seem incompatible.

[billsacks]

Note that accumulMod routines currently can't be called from inside a clump (threading) loop. I believe that it would be very difficult to remove that limitation until we rework threading to be done at a higher level. This is because the accumulMod infrastructure is general enough to handle fields at any level (gridcell, column, patch, etc.), and if you want to have code inside a clump loop, you need to declare the lower bounds of any array arguments... these two things seem incompatible.

Looking back at this: I think we could accomplish this in a way similar to what's done in AnnualFluxDribbler.F90. Specifically: We could pass the bounds object to various subroutines in accumulMod, then access the appropriate bounds via calls to the generic routines, get_beg and get_end.

This seems worth doing so that we can put accumulMod routines - specifically, the update routines that are called every time step - inside clump loops. This will also allow us to call these update routines from places in the code where they fit more naturally, rather than calling them all at once near the end of the driver loop (which is needed now in order to avoid having them inside a clump loop).

[billsacks]

Another change that should be made is better error-checking on field names in init_accum_field:

1. We should ensure that the passed-in field name isn't too long (currently the maximum string length is 8 characters)

2. We should ensure that the requested field name doesn't already exist.

However, given this comment https://github.com/ESCOMP/CTSM/issues/30#issuecomment-352208214 , the name actually becomes much less important; the important thing is just ensuring that we don't try to write two restart fields with the same name. If we address that comment, we would presumably remove the find_field routine, and so we could ignore the idea of ensuring that the requested field name doesn't already exist.

[billsacks]

We should also convert accumulMod to being truly object-oriented, as noted in this comment:

https://github.com/ESCOMP/CTSM/blob/99a32659a700b0a97e6a6ca1b3118a35c1a8a20f/src/main/accumulMod.F90#L86-L90

[billsacks]

Another thing to do: In init_accum_field, add error checking: if numlev > 1 but type2d is absent, abort.