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

Lower number-density termination threshold #189

Merged
merged 6 commits into from
Mar 14, 2017
Merged

Lower number-density termination threshold #189

merged 6 commits into from
Mar 14, 2017

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Feb 24, 2017

Fixes to termination logic to allow demotion to occur successfully

reduces termination threshold due to small number density and also deletes calls to terminate_cohorts in CanopyStructureMod except for those that are after a call to fuse_cohorts.

this is on top of the code in PR #184, so will shrink to just ten or so lines of code once that is merged.

Fixes: #187

User interface changes?: No

Code review: changes done in consultation with @rgknox and @rosiealice

Test suite: ed tests on lawrencium
Test baseline: a5dc8da
Test namelist changes:
Test answer changes: climate changing

Test summary: [Remove 'PASS' fields]

[cdkoven@n0002 ed.lawrencium-lr3.intel.37ba5ce]$ ./cimeteststatus | grep FAIL
FAIL SMS_D_Lm6.f45_f45.ICLM45ED.lawrencium-lr3_intel.clm-edTest.clm2.h0.nc : baseline compare clm2.h0 (baseline: compare .base file with ed.lawrencium-lr3.intel.a5dc8da file)
FAIL SMS_D_Lm6.f45_f45.ICLM45ED.lawrencium-lr3_intel.clm-edTest.cpl.hi.nc : baseline compare cpl.hi (baseline: compare .base file with ed.lawrencium-lr3.intel.a5dc8da file)
FAIL SMS_D_Lm6.f45_f45.ICLM45ED.lawrencium-lr3_intel.clm-edTest.rtm.h0.nc : baseline compare rtm.h0 (baseline: compare .base file with ed.lawrencium-lr3.intel.a5dc8da file)
FAIL SMS_D_Lm6.f45_f45.ICLM45ED.lawrencium-lr3_intel.clm-edTest : baseline compare summary (baseline: compare .base file with ed.lawrencium-lr3.intel.a5dc8da file)

@rgknox
Copy link
Contributor

rgknox commented Feb 25, 2017

It does make sense that we would only start to see answer changing results on the LM6 run.

@ckoven
Copy link
Contributor Author

ckoven commented Feb 25, 2017

why—is that the long one?

@rgknox
Copy link
Contributor

rgknox commented Feb 25, 2017

yeah, I figure the six month test is a long enough simulation for the termination mortality to really take effect. The other tests are only a few days at most, we simply don't have enough growth/mortality cycles to effect plan number densities by then.

@ckoven
Copy link
Contributor Author

ckoven commented Mar 10, 2017

retested on lawrencium after merging; same results as before. @rgknox, now that this PR contains only the answer-changing code, could you take another look to make sure that what is there is what is intended to be there, run the rapid science check, and then assign to @bandre-ucar to test on ucar machines and pull?

@rgknox
Copy link
Contributor

rgknox commented Mar 13, 2017

I performed a review of the code to see if it makes sense to me, and it makes sense to me. The changes seem to address what was intended.

I also ran a Rapid Science Check (RSC) and the results are as expected. Since @ckoven changes are expected to be science changing, I was anticipating small but not-insignificant divergence from master, and that was what was generated. See below for a 1x1 brazil:

bdead_threshold
ncohort_threshold
cfluxes_threshold
fluxes_diurnal_threshold

Make note of the changes in maintenance respiration. Note that while GPP stayed roughly the same, maintenance respiration responded, presumably somehow to the decreased frequency of "demotion termination".

@rosiealice
Copy link
Contributor

rosiealice commented Mar 13, 2017 via email

@rgknox
Copy link
Contributor

rgknox commented Mar 13, 2017

since these passed lawrencium already, passing this on to @bandre-ucar for testing on yellowstone and friends

@rgknox rgknox assigned bandre-ucar and unassigned rgknox Mar 13, 2017
@rgknox rgknox self-requested a review March 13, 2017 21:00
Copy link
Contributor

@rgknox rgknox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passes review, see thread

@bandre-ucar
Copy link
Contributor

running tests

@bandre-ucar bandre-ucar merged commit 07c10f3 into NGEET:master Mar 14, 2017
@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.

4 participants