Fixes: carbon flux naming, messages and warning, thread-change test #147
Conversation
…n included. One free gpp with any resp of equal or lesser value.
…lution was straightforward.
…s tripping true for all restart initialization cases and producing false positive warnings. I changed the logic to only trip in cases where the arguments passed into the routine signaled something was unusual, and in that case to kill the run instead of simply report.
|
Updated this branch with current master, re-tested, test status remains the same, all pass (except new thread-test). |
|
bump |
| @@ -722,6 +722,9 @@ | |||
| </test> | |||
| </grid> | |||
| <grid name="f10_f10"> | |||
| <test name="ERP_D_P15x2_Ld5"> | |||
| <machine compiler="ed" testtype="ed" testmods="clm/edTest">ed</machine> | |||
| </test> | |||
There was a problem hiding this comment.
From the PR description, this test probably won't pass on any machine. Can we remove it from this PR and put it on a branch for threading debugging?
Doing TDD with a failing test is great, but it causes problems when it is on the master prematurely. Adding a test before it works means everyone who runs the test suite has to deal with the failure. Historically this has been very frustrating for CLM scientists because they don't keep up with development enough to know that some tests are expected to fail or how to tell which ones are suppose to fail. This leads them to spend time debugging a failure that they didn't cause or get frustrated by the false positive and not trust the test suite. Then people get desensitized to failures and start ignoring them.
If we keep it on master, then the PR needs to be updated with this test added to the expected failures file: components/clm/cime_config/testdefs/ExpectedTestFails.xml
There was a problem hiding this comment.
Sure thing, I will remove this test from the PR and update. We can add this test on to whichever future PR generates a pass.
…st starts showing passes.
|
Thread change test was removed. |
|
@rgknox Thanks for updating. Yellowstone is down, but I'll start testing when it comes back up. |
| currentCohort%resp_acc_hold = (currentCohort%n*currentCohort%resp_acc_hold + nextc%n*nextc%resp_acc_hold)/newn | ||
| currentCohort%npp_acc_hold = (currentCohort%n*currentCohort%npp_acc_hold + nextc%n*nextc%npp_acc_hold)/newn | ||
| currentCohort%gpp_acc_hold = (currentCohort%n*currentCohort%gpp_acc_hold + nextc%n*nextc%gpp_acc_hold)/newn |
There was a problem hiding this comment.
NAG build failure:
Error: /home/andre/work/ed/pr147/ed-clm/components/clm/src/ED/biogeochem/EDCohor
tDynamicsMod.F90, line 722: Implicit type for N
detected at N@<end-of-statement>
Error: /home/andre/work/ed/pr147/ed-clm/components/clm/src/ED/biogeochem/EDCohor
tDynamicsMod.F90, line 725: Implicit type for NEW
detected at NEW@<end-of-statement>
The fortran standard specifies that lines may not exceed 132 characters. NAG enforces this and it can not be worked around. Please review the changes in this PR and ensure that none of the lines exceed 132 characters.
|
Nag should be able to read that section of code now. |
| EDecophyscon%leaf_stor_priority(currentCohort%pft) | ||
|
|
||
| currentCohort%carbon_balance = currentCohort%npp - currentCohort%md * EDecophyscon%leaf_stor_priority(currentCohort%pft) | ||
| currentCohort%carbon_balance = currentCohort%npp_acc_hold - currentCohort%md * EDecophyscon%leaf_stor_priority(currentCohort%pft) |
There was a problem hiding this comment.
This line also exceeds 132 characters.
…imum line length to the eddi machine settings to help catch these before I submit.
|
This time I added maximum line length on the eddi machine to help me catch these things before I submit. I hope this time there are no more long line culprits. |
|
@bandre-ucar you too. Thanks for you patience on those line length exceedences |
|
@rgknox I'll resolve the conflict and test and merge. |
This PR adds a group of minor fixes and additions. The change group, with a minor exception should generate bit-for-bit results.
The first change is the renaming of cohort level carbon flux accounting variables %npp to %npp_acc_hold, with documentation explaining the change, see #134.
The second change is the fix on a warning message when cohorts are created during restarts. The restart was passing arguments to cohort creation that were triggering a warning because the values were un-expected. Seeing how these statically set arguments are as expected as an argument can be, I removed the evaluation of those variables from the logic that triggered the warning, and changed the warning to a fail. I also but debug logic around two other print statements that were generating a lot of log output. See #133.
The third change is the addition of a thread-change ERP restart test. This test being introduced in this PR, has no baseline, so no B4B testing is expected. Note that this test does not generate a completed run on lawrencium, so I'm glad we are getting it in there! I believe the crash occurs on the restart portion of the test, during the first call to do vector io in fates. See #132. NOTE: This change has been punted to a later date when the tests start showing pass.
Fixes: #134,#133
User interface changes?: no
Code review:
Test suite: lawrencium-lr3 intel: edTest, clm_short45, clm_short50
Test baseline: 53bbb9d
Test namelist changes: none
Test answer changes: b4b
Test summary: all PASS except ERP_D_P15x2_Ld5.f10_f10 (new test, should now be flagged as expected fail)