search

ESCOMP / CTSM

Community Terrestrial Systems Model (includes the Community Land Model of CESM)
http://www.cesm.ucar.edu/models/cesm2.0/land/
Other
310 stars 314 forks source link

rework or remove max_patch_per_col #37

Open billsacks opened 6 years ago

billsacks commented 6 years ago

Bill Sacks < sacks@ucar.edu > - 2015-10-04 14:41:13 -0600 Bugzilla Id: 2227 Bugzilla CC: andre@ucar.edu, rfisher@ucar.edu,

Currently in clm_varpar, we have:

max_patch_per_col= max(numpft+1, numcft, maxpatch_urb)

This is used in loops in the code like this:

do pi = 1,max_patch_per_col
   do j = 1,nlevsoi
      do fc = 1, num_hydrologyc
         c = filter_hydrologyc(fc)
         if (pi <= col%npatches(c)) then

However: Using numcft in this 'max' gives a significant overestimate of max_patch_per_col when use_crop is true. This should be reworked - or, better, removed from the code entirely (because it is a maintenance problem, and I can't imagine that looping idioms that use it help performance that much, and likely they hurt performance - at least when it is overestimated by so much.)

Loops like this could be reworked to avoid needing max_patch_per_col by either:

(1) Looping over patches, and finding the corresponding column with patch%col(p)

(2) Looping over columns, then looping from begp to endp in an inner loop. This would likely be less vectorization-friendly, but you save many unnecessary loop iterations, so it's probably not likely to hurt performance much, and may help it.

billsacks commented 6 years ago

Bill Sacks < sacks@ucar.edu > - 2015-10-05 15:51:11 -0600

Ben (or was it Erik?) points out: in the calculation of max_patch_per_col, we could replace numcft with something like num_crop_patches_per_col, which would be 1 for a prognostic crop run.

I'm going to defer this change until after we pass the crunch time for the upcoming science freeze.

billsacks commented 4 years ago

Slightly related?: #881

ekluzek commented 4 years ago

@billsacks and I talked about this some a few days ago. And @negin513 and I looked into this issue yesterday. This would be good to do, because it's strange to have more than one way to loop over patches. Bill and I thought maybe this was just restricted to the fire code, but it's not. In the latest code it's used in the following places...

biogeochem/CNCIsoFluxMod.F90 biogeochem/CNPhenologyMod.F90 biogeophys/SoilFluxesMod.F90 biogeophys/SoilTemperatureMod.F90 biogeophys/SoilWaterMovementMod.F90 biogeophys/SoilWaterPlantSinkMod.F90

ekluzek commented 4 years ago

@negin513 and I went over this. Using option 1, this is what needs to happen...

For example in CNCisoFluxMod the following type of change happens...

Replace the pi and fc loops with...

     do fp = 1, num_soilp
        pp =  filter_soilp(fp)
        cc =  patch%column(pp)

then remove the previous setting of pp and cc, as well as the if statement... if ( pi <= col%npatches(cc) ) then

The names of the indices will depend on the specific piece of code that's being changed.

This removes a nested if statement as well as an evaluation of an if statement nested inside the loop structure. So it makes the code easier to read as well as using a consistent looping structure (most of the loops in CTSM are structured the way illustrated by the construct above rather than the few that use the max_patch_per_col construct. so it improves both readability and consistency as well as liking improving performance of these loops.