Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FATES interface: conversion to columnization #72

Merged
merged 24 commits into from
Jun 14, 2016
Merged

FATES interface: conversion to columnization #72

merged 24 commits into from
Jun 14, 2016

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Jun 7, 2016

In this PR, FATES sites are converted from alignment with gridcells, to alignment with columns in the host land model. Sites are still connected to the fates(nc)% structure, although syntactically they are now just called "sites(:)" instead of "ed_allsites_inst(:)". eg clm_fates%fates(nc)%sites Some other notable design constructs:

  1. sites is allocated for each column on the natural vegetation land-unit. @billsacks and I have discussed other ways to filter this further, and have decided that allowing FATES sites to exist on all columns, even ones covered by ice or ones that have no weighting, while not ideal, is not a liability to getting correct results, and is not an immense performance hit. Future commits and issues should be brought up to strategize how to allocate and filter FATES sites for only active columns.

  2. sites is not sparse, and is allocated from 1 to clm_fates%fates(nc)%nsites. As mentioned above, since it is anchored in the fates(nc)% structure, the vector space is separated by thread.

  3. two mapping vectors have also been created:
    clm_fates%f2hmap(nc)%fcolumn(1:nsites) this vector returns the column index on each clump, associated with the FATES site index
    clm_fates%f2hmap(nc)%hsites(bounds_clump:begc:bounds_clump:endc) this vector returns the FATES site index associated with a given HLM column index. Zero's imply no FATES site, and this is sparse. This is almost always called from within a filtered loop on the HLM side, so there is no need to check if the column is non-zero, although there is a check in the code with a fail call.

  4. restarts and history writes appear to be working correctly. Note that FATES uses the cohort level memory space and the column level memory space in the HLM IO machinery, and not any patch level space. The cohort vector space that is allocated is max_number_of_patches_per_col * max_number_of_cohorts_per_patch. This vector is incredibly sparse, and is also something that needs to be addressed, still. There are here: 1) currently the cohort IO vector space is allocated for all columns, and not just columns on naturally vegetated land units. 2) there are various variables that use the cohort IO vector space to hold their information, which is forcing us to use a very large max number of cohorts.

  5. We are still using a variable that maps the FATES patch to its associated HLM patch index: currentpatch%clm_pno. This seems inconsistent with the library design. I wanted to remove it in this pass, but held off. Alternatively, there were indeed several variables at the site level that pointed towards the CLM/ALM gridcell, these have been removed.

  6. Cosmetic improvements to the code and updated annotation is still needed in various places.

Fixes: #66, #30

User interface changes?: No. But, people will need to update their analysis codes to use the new IO variables. For history output, the only variables that were changed were all of the "scpf" variables, that had been indexed by gridcell, they are no column variables. For restarts, we now have ed_io_numPatchesPerCol (instead of ed_io_numPatchesPerGCell, or something like that), and the variable that indicated whether or not the column has a patch has been removed, as that information is redundant with ed_io_numPatchesPerCol.

Code review: Discussion with @ckoven @rosiealice, @bandre-ucar, @billsacks and Mariana V. A check-in, discussion and/or commentary from: @bishtgautam and @climate-dude are welcomed as well.

Formal code evaluation TBD:

Test suite: edTest, lawrencium-lr3, intel
Test baseline: none, output vectors have changed, regression tests should not work. however see in #66 that the RSC tool was used to evaluate science output, and results were identical in the 1x1_brazil.
Test namelist changes: none
Test answer changes: See baseline explanation

Test summary:

DONE ERS_D_Ld5.f19_g16.ICLM45ED.lawrencium-lr3_intel.clm-edTest : (test finished, successful coupler log) 
     --- Test Functionality  ---:
    FAIL ERS_D_Ld5.f19_g16.ICLM45ED.lawrencium-lr3_intel.clm-edTest.clm2.h0.nc : test compare clm2.h0 (.base and .rest files) 
    FAIL ERS_D_Ld5.f19_g16.ICLM45ED.lawrencium-lr3_intel.clm-edTest.cpl.hi.nc : test compare cpl.hi (.base and .rest files) 
    FAIL ERS_D_Ld5.f19_g16.ICLM45ED.lawrencium-lr3_intel.clm-edTest : test functionality summary (ERS_test) 

DONE ERS_D_Ld5.f09_g16.ICLM45ED.lawrencium-lr3_intel.clm-edTest : (test finished, successful coupler log) 
     --- Test Functionality  ---:
    FAIL ERS_D_Ld5.f09_g16.ICLM45ED.lawrencium-lr3_intel.clm-edTest.clm2.h0.nc : test compare clm2.h0 (.base and .rest files) 
    FAIL ERS_D_Ld5.f09_g16.ICLM45ED.lawrencium-lr3_intel.clm-edTest.cpl.hi.nc : test compare cpl.hi (.base and .rest files) 
    FAIL ERS_D_Ld5.f09_g16.ICLM45ED.lawrencium-lr3_intel.clm-edTest : test functionality summary (ERS_test) 


rgknox and others added 24 commits May 23, 2016 18:01
…tor to list procedures. Currently on convertCohortVectorToList().
…e colWithPatch was removed because it is offers no information beyond what is provided by numPatchesPerCol (which is more information rich. Also, old_stock,cd_status,dd_status,ncd,leafondate,leafoffdat,dleafondate,dleafoffdate and acc_NI are stored in the restart as column indexed, as they are site level variables. If these variables are converted to patch-scale, then they need large restart vector allocations.
…umn status. 1x1br cold-starts running with what appears to be non-nonsense results.
… on inactive columns on naturally vegetated landunits
…tional to prevent prevent passing of unallocated arrays
… (bug from previous commit). Updated CanopySunShadeFracs to be called from clm_fates, and also fixed how that function accesses the correct site.
…ccident I got all excited and started flushing patch pointers before patches had been initialized. I de-wronged it.
@ckoven
Copy link
Contributor

ckoven commented Jun 7, 2016

@rgknox does this also fix #30?

@rgknox
Copy link
Contributor Author

rgknox commented Jun 7, 2016

I believe so.

We currently generate FATES sites during clm_fates%init_allocate(), like so:

s = 0
         do c = bounds_clump%begc,bounds_clump%endc
            l = col%landunit(c)

            ! These are the key constraints that determine if this column
            ! will have a FATES site associated with it

            if(DEBUG)then
               write(iulog,*) 'clm_fates%init(): thread',nc,': found column',c,'with lu',l
               write(iulog,*) '  LU type:', lun%itype(l)
            end if

            ! INTERF-TODO: WE HAVE NOT FILTERED OUT FATES SITES ON INACTIVE COLUMNS.. YET
            ! NEED A RUN-TIME ROUTINE THAT CLEARS AND REWRITES THE SITE LIST

            if (lun%itype(l) == istsoil ) then
               s = s + 1
               collist(s) = c
               this%f2hmap(nc)%hsites(c) = s
            endif

         enddo

The condition to be met is that the landunit of the column is "istsoil" (which is the natural vegetation LU). Note that there is only 1 column on that LU right now, so depending on how that changes, we could react. Above, we are marking down the columns that meet that criteria. And as the comments say, we are not ignoring inactive columns.

You bring up a question in that other issues thread:

"note: to be clear, I think all I am asking is whether there exists a column filter that corresponds to the set of columns described by ed_allsites_inst(:)%clmcolumn. "

clmcolumn is now deprecated, but we do now have a mapping vector that achieves the same goal, just not part of the fates(nc)% structure. It is in a new structure called f2hmap(nc)%fcolumn. And to answer your question, we do have a column filter that points to the FATES sites, called f2hmap(nc)%hsites (which is allocated bounds_clump:begc:bounds_clump:endc).

I believe #30 is addressed (I mean gee that was kinda the point of the PR right, so I hope so). I do believe the code is consistent on which columns and patches FATES operations are conducted on. This is because the mapping tables fcolumn and hsites are created in one place, and FATES to HLM matching now always happens through those.

I think the thing that I am most weary of, is where we need to do things like zero non FATES stuff. We also use those mappings to create an is_veg flag, which is also used to control some loops, which needs to be addressed.

TLDR, I think we can close that issue if we are able to sign off on the PR.

@ckoven
Copy link
Contributor

ckoven commented Jun 8, 2016

roger that.

@bandre-ucar
Copy link
Contributor

tests are running on yellowstone

@bandre-ucar bandre-ucar merged commit 84d289f into NGEET:master Jun 14, 2016
@rgknox rgknox deleted the rgknox-fatescolumns branch August 5, 2016 23:52
@bandre-ucar bandre-ucar removed their assignment Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants