current code not conserving primary/second land areas

[ckoven]

the current code isn't conserving primary and secondary land areas, for reasons I believe having to do with the forced fusion of very small patches. I encountered this in a transient historical run with land use, but since that is a new capability which is uncovering a bug that exists in the prior code, I wanted to make an issue in case anyone else is running with secondary forest dynamics in the old harvest logic.

i made a change to the new patch insertion logic that seems to fix the bug, at least in the case where I encountered it. basically, rather than adding newly-disturbed patches to the youngest side of the patch linked list irrespective of their land-use label, which results in interleaved primary and secondary patches through the list, I changed it to add the newly-disturbed primary patch after all of the secondary patches, if there are any. This results in contiguous primary and secondary patches in the linked list, which then avoids the force-merge bug. I also have added a logical check to fail if two patches with different labels are being fused, in case there are other edge cases that I haven't run into yet.

@rosiealice I'm not sure if this logic might apply to your pft labeling case in the prescribed biogeography mode with disturbance too?

proposed fix here: ckoven@0b6ab98

[rosiealice]

Interesting. I haven't been using the secondary forest capabilities at all yet in the NOCOMP mode, but I did have a bunch of problems with the forced fusion of v v small patches in terminate cohorts (I was triggering the 'you only have one patch like this and it is tiny' check).

To that end, as i mentioned earlier, I remove the very small patches (<1%) in the initialization and distribute their area to the other present PFTs ( in fixed biogeog mode).

Having said all of that, I do still have a memory leak or something when I try and turn on my implementation of patch dynamics with no competition. I am essentially looping round the PFTs and creating one primary and one secondary patch, starting here:
https://github.com/rosiealice/fates/blob/ea3c635d00e7b3863b69d75ffb555c7617eb8f93/biogeochem/EDPatchDynamicsMod.F90#L489

and ending here
https://github.com/rosiealice/fates/blob/ea3c635d00e7b3863b69d75ffb555c7617eb8f93/biogeochem/EDPatchDynamicsMod.F90#L1093

and so we end up with patches that aren't able to fuse with each other interleaved all over the place! Obviously they need to not fuse together, as per:
https://github.com/rosiealice/fates/blob/ea3c635d00e7b3863b69d75ffb555c7617eb8f93/biogeochem/EDPatchDynamicsMod.F90#L2248

but I also had to add a whole bunch of logic to terminate_patches to search for the other patches belonging to that PFT in an attempt to try and find another patch to fuse our tiny patches to....
https://github.com/rosiealice/fates/blob/ea3c635d00e7b3863b69d75ffb555c7617eb8f93/biogeochem/EDPatchDynamicsMod.F90#L2577

which I believe might be an alternative fix to the problem you encoutered and fixed by ordering the primary and secondary patches to always group together??

I think this code works, but the memory usage skyrockets right from the start of the simulation, so something is wrong with my allocate statements or pointers or something.

[rosiealice]

p.s. It is a cruel irony that we've both been fighting the same problem from different angles!

[ckoven]

@rosiealice yeah that's what I was afraid of. it seems like there ought to be a way of generalizing this logic, since both cases boil down to making categorical rather than continuous distinctions between patches. what if we make some sort of generic categorical index along the lines of patch_category = (i_landuse_type -1) * n_pft_label + i_pft_label that could in principle be extended to different types of categorical distinctions if we want to add them, and use that for this type of logic? it may not get us very far with only 14 patches but can open things up for once we remove that cap.

[ckoven]

thinking my last point through a bit more, I guess that approach could help to simplify the logic in the patch fusion routines, like if we changed from the primary/secondary index to the to a combined index here: https://github.com/rosiealice/fates/blob/ea3c635d00e7b3863b69d75ffb555c7617eb8f93/biogeochem/EDPatchDynamicsMod.F90#L2172 would then let you get rid of the logic here: https://github.com/rosiealice/fates/blob/ea3c635d00e7b3863b69d75ffb555c7617eb8f93/biogeochem/EDPatchDynamicsMod.F90#L2249 ?

But I guess it wouldn't really help simplify the disturbance routine, since you'd still need to iterate over the pft labels (as you've done there in https://github.com/rosiealice/fates/blob/ea3c635d00e7b3863b69d75ffb555c7617eb8f93/biogeochem/EDPatchDynamicsMod.F90#L1093) separately from the primary/secondary.

so not sure whether or not the overhead is worth it?