Dynamic Patch Arrays - larger nclmax and nlevleaf #1198
Conversation
rgknox
changed the title
|
There are a few patch level variables that have weird names I'd also like to change: patch%ncl_p : The _p part indicates patch, but that is implied by being on the patch structure, I'd like to change this so "ncan" patch%ncan(:,:) : This is not the number of canopy layers! Its the number of vegetation layers in each canopy by pft class! I'd like to change this to "nveg(:,:) patch%nrad(:,:) : This should be removed, we don't use it! |
| else | ||
| currentPatch%NCL_p = z | ||
| end if | ||
|
|
There was a problem hiding this comment.
It is nice to have the informative error message, but I wonder if error as opposed to cohort termination is something we always want to do from now on. My only concern is if this could trigger too many error messages in global runs or parameter sensitivity experiments.
But we can see if this becomes a problem and address if needed.
There was a problem hiding this comment.
I'm open to allowing termination here. I suppose it will allow the model to continue working when people are testing strange edge-case parameter combinations that generate more than 5 canopy layers.
There was a problem hiding this comment.
@mpaiao , I looked at this again. The calls to termination that precede this section should be ensuring that z <= nclmax. If z is larger than nclmax it should be in error at this point.
|
I compared the timing output for simulations at BCI that use this method and the old method using a cap of 3 canopy layers. The simulations had no noticeable difference in run time. |
Bug fix for non-land use run modes
|
Regression testing underway on |
|
Regression testing against The The The lnd.log file doesn't provide much context aside from the fact that it failed about 2/3 of the way through the initial case. Test results can be found: @rgknox there are DIFFs, which I think are as expected, although I only spot checked a few. That said, I'm going to rerun the baseline to make sure that the one I was comparing against was generated correctly. |
It looks like the old baseline I had tested against was not with the latest tags. I've moved that one and regenerated |
|
I realized I had missed in the |
The updated run with corrected baseline comparison can be found here: The number of DIFFs has reduced, although it's a little difficult to discern at first as there are 45 DIFFs reported that only have to do with the dimensions differing, which is as expected. The list of cprnc.out files with DIFFs that actually have non-zero differences can be found in the |
|
@glemieux , are there still run fails? |
|
@rgknox yes I'm still seeing the same failures as noted here: #1198 (comment) |
|
Retesting with |
|
Regression testing the |
|
Still working out why there is a failure in the ERS_P128x1_Lm25.f10_f10_mg37.I2000Clm60Fates.XX.FatesColdNocomp tests. I've tried making it a debug test and running on gnu. The model failed at different places, the common thread seems to be related to heat/energy/temperature. This makes me suspect something isnt getting zero'd vis-a-vis radiation arrays... I also tried removing the NoComp specification, ERS_D_P128x1_Lm25.f10_f10_mg37.I2000Clm60Fates.derecho_gnu.clm-FatesCold does pass... |
|
@mpaiao @glemieux and other reviewers: ERS_P128x1_Lm25.f10_f10_mg37.I2000Clm60Fates.XX.FatesColdNocomp fails with main when I up the nclmax to 3. So I don't believe the problem is with this pull request, main "should" pass this test when nclmax = 3. I propose setting nclmax = 2 in this pull request, integrating following a re-do of tests, and then creating an issue. I'm happy to prioritize the issue. |
|
tests look good, b4b with base: /glade/derecho/scratch/rgknox/tests_0731-094347de |
|
Tests look good except for the PVT baseline, which is not passing for other tests as well. /glade/derecho/scratch/rgknox/tests_0813-195403de |
Description:
This set of changes makes a bunch of arrays attached to the patch structure dynamically allocated. Most of these arrays are dimensioned by: number-of-canopy-layers x number-of-pfts x number-of-veg-layers.
Previously, in the interest of keeping these arrays small, we had elected to keep nclmax equal to 2 and nlevleaf = 30, and manually increase this value when necessary. We still have these two constants, but they are used to either allocate stack space, or smaller dimension arrays, so we can bump them up to numbers that won't affect peoples runs.
So now, nclmax = 5 and nlevleaf = 50. We could bump them up higher if need be, I'm keeping them low because they still affect the size of history output arrays. But, users can set the history dimensionality to level 1 to get rid of large arrays in their output anyway.
Collaborators:
This has been a refactor target for years, so lots of people have probably weighed in.
Expectation of Answer Changes:
No answer changes but... the canopy structure part of the code is super sensitive to order of operation changes, so I do actually expect very small diffs.
Checklist
If this is your first time contributing, please read the CONTRIBUTING document.
All checklist items must be checked to enable merging this pull request:
Contributor
Integrator
Documentation
Test Results:
CTSM (or) E3SM (specify which) test hash-tag:
CTSM (or) E3SM (specify which) baseline hash-tag:
FATES baseline hash-tag:
Test Output: