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

interface task: switching FATES sites to match columns #66

Closed
rgknox opened this issue May 23, 2016 · 3 comments
Closed

interface task: switching FATES sites to match columns #66

rgknox opened this issue May 23, 2016 · 3 comments
Assignees

Comments

@rgknox
Copy link
Contributor

rgknox commented May 23, 2016

Summary of Issue:

FATES sites are currently aligned with the grid index of the host model (ALM/CLM). We have identified that a more natural match is that sites are aligned with columns.

I will go ahead and start poking the code and see if I can migrate to columns. I have done a fair bit of exploratory work on how to refactor restarts and the packing-boundary conditions, and I believe that these works are bottle-necked until we migrate to sites aligned with columns. Since I will be modifying ed_allsites_inst() across much of the FATES code, I will also remove the propogation of bounds as its indexing into the FATES code.

Design Specification: fates(nc)%sites(:) will be allocated and indexed from 1 to nsites

Changeset ID of my base: 4b2b970

@rgknox rgknox self-assigned this May 23, 2016
@rgknox
Copy link
Contributor Author

rgknox commented May 25, 2016

working branch can be found here:

https://github.com/rgknox/ed-clm/tree/rgknox-fatescolumns

@rgknox
Copy link
Contributor Author

rgknox commented Jun 2, 2016

Here is a little status update:

I have a working running version now that uses columns.

The big caveat is that there is an assumption that the column on the naturally vegetated land-unit is active. The reason for this is that the active flag has not been established at the point in the initialization sequence where we need to start allocating FATES sites.

As the host land models (HLMs) start to use more dynamic column management, we need to decide how we are going to flush or remove FATES sites. One choice is to allocate FATES sites on all active and non active columns associated with naturally vegetated landunits, and then use filters to dictate which associated FATES sites are active. Another choice is to actively allocate and deallocate our sites when the active in-active flags switch. (the existing code went with method 1 as far as I can tell)

I suppose from a biological perspective, that if a column becomes inactive (for reasons of ice/crop invasion, etc), that the state of the canopy at that time need not be remembered. So for all intents and purposes, we should flush the patches and cohorts associated with a FATES site when that happens.

My current testing is with the Brazil 1x1 grid (single site). Once tests at this site lead me to believe things are "working", I will need to move on to global gridded cases, at which point a decision will have to be made about how to handle non-active sites. I think this is true even if their is not dynamically active sites, as the active status flags at the start of the simulation may influence the results.

Here are some early tests, which show for the most part, over a 25 year simulation, that the "columnized" version of the code (test) matches master (base). However, I have also been running into IO errors when running long simulations that produce high-frequency (sub daily) history writes, in both master and test branch. I should make an issue of this. As an artifact of this, the test simulation shown uses debug flags and the master (base) did not, don't ask why this is so, I know its not appropriate behavior.

ed_restvars_1x1br_57c533c_vs_6a64c0c

ed_restvars2_1x1br_57c533c_vs_6a64c0c

@rgknox
Copy link
Contributor Author

rgknox commented Jun 3, 2016

Update: I just remembered that I had introduced a minor fix to remove a div0 in commit 6a64c0c of the feature branch. This was the reason for the slightly different results shown above. I went ahead and added those changes in master, re-did the comparison, and the test branch now seems to reproduce the master branch with un-noticable differences. See below (it also seems to be fixing some problems I was having with history output crashing with high frequency writes, although I am not sure if the div0 fix is the best fix).

I will now start working out how to filter fates sites based on active inactive column status, and then test regionally. Feed-back or discussion on this would be helpful.

ed_restvars1_1x1br_42a74d8_vs_6a64c0c

ed_restvars2_1x1br_42a74d8_vs_6a64c0c

bandre-ucar added a commit that referenced this issue Jun 14, 2016
Merge branch 'rgknox-fatescolumns'

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?: Yes - 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 Vertenstein.

Test suite: ed - lawrencium-lr3 intel; yellowstone gnu, intel, pgi

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: Pass, expected failures #14 f09 and f19 restart. #43 f10
gnu restart on yellowstone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant