duplicate variables: cohort%gpp and cohort%gpp_acc #134
Comments
|
So, each timestep we get a different GPP/NPP/resp (*_timestep) and those So, I think we need something like npp_acc to remember what happened over Apologies if I missed something obvious etc... On 10 October 2016 at 17:10, Ryan Knox [email protected] wrote:
Dr Rosie A. Fisher Terrestrial Sciences Section |
|
It sounds like we are in agreement as to the need and value of the _acc variables, they are important. As are the _tstep variables. But it seems that we both have not been able to identify a purpose for cohort%npp, cohort%gpp and cohort%resp. They don't really do anything right? Do we need to keep them? |
|
aren't they needed if you want to output the data at sub-daily frequency? |
|
As far as I can tell*, %npp %gpp and %resp all do nothing that can't be accomplished by %npp_acc, %gpp_acc and %resp_acc The %npp variable is used in growth_derivatives to calculate the npp partions, but in this case we could easily use %npp_acc times the scalar. It is then used again in history IO to write diagnostics, and again npp_acc would suffice. %gpp is only used for history IO, and also %gpp_acc could be used instead %resp is also only used for history IO, and %resp_acc could be used instead *I've looked through this several times, I'm pretty sure we don't need these variables |
|
right. looked through the code and see your point. at history writes, the sub-daily variables are being given %npp_tstep, %gpp_tstep, and %resp_tstep; whereas %npp and %gpp are being used for the daily, disaggregated fluxes on the scpf dimension. I don't see %resp going into the history at all (on master), only %resp_tstep and its growth and maintenance components. anyway, agreed that they are redundant so we ought to get rid of them. |
|
I'm cool with getting rid of the gpp, npp, & resp variables too... On 12 October 2016 at 10:25, Charlie Koven [email protected] wrote:
Dr Rosie A. Fisher Terrestrial Sciences Section |
|
Removing these variables is a little trickier than I first thought, and I'm starting to think that they are actually quite necessary. :| Our history diagnostics that occur after the call to dynamics wants integrated npp, gpp. In the new proposed system, instead of using %npp and %gpp, we use the %npp_acc and %gpp_acc variables and just scale the values (having removed the variables npp, gpp which were scaled copies of npp_acc and gpp_acc). The problem is, that we call an update on history diagnostics on the initialization of restarts. But npp_acc and gpp_acc had been zero'd prior to being written out to the restart file. In the old system, we don't zero %npp, %gpp and %resp (as they are not integrated), so we don't have this problem. And this call to update the dynamics history buffers are necessary, because they won't be set with new information again until the end of the day, and those buffers must match what they looked like before restart data was written out. |
|
I intend to close this up and make no changes per this thread unless anyone else feels otherwise. I will give it a few days. |
|
ok. maybe it would be worthwhile to at least (a) add documentation in the code about what these variables are needed for so that future people don't have to re-learn what you've discovered? and (b) change the variable names of npp, gpp, and resp to be a little more descriptive of how they are used in the code? like npp_daily or something like that? |
|
excellent idea |
Merge branch 'rgknox-warns-threadtests' 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. Remove the evaluation of those variables from the logic that triggered the warning, and changed the warning to a fail. Added debug logic around two other print statements that were generating a lot of log output. See #133. Fixes: #134, #133 User interface changes?: no Code review: rgknox: 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 andre: Test suite: ed - yellowstone gnu, intel, pgi ed - hobart nag Test baseline: 1d7f88a Test namelist changes: none Test answer changes: bit for bit Test summary: all tests pass Test suite: clm-short - yellowstone gnu, intel, pgi Test baseline: clm4_5_12_r195 Test namelist changes: none Test answer changes: bit for bit Test summary: all tests pass
Summary of Issue:
We have a variable that essentially a constant scale of the other.
gpp_tstep, npp_tstep and resp_tstep are calculated as rates in Photosynthesis. Photosynthesis is a rapidly updated process at every model time-step. Following photosynthesis, we call an accumulator function that temporally increments these three variables, and zeros these values following the call to dynamics (once daily).
gpp_acc = gpp_acc + gpp_tstep
The accumulation of the instantaneous variable is all well and good, and I don't see any issues with that. I just don't like how we have a separate variable that is just a scaled version of the accumulated variable. I think it is much clearer to have one variable instead of two that do the same thing.
in EDPHysiologyMod.F90, line 792:794:
I just want to get rid of %npp, %gpp and %resp. Does this make sense? Or am I missing something?
Expected behavior and actual behavior:
Steps to reproduce the problem (should include create_newcase or create_test command along with any user_nl or xml changes):
What is the changeset ID of the code, and the machine you are using:
have you modified the code? If so, it must be committed and available for testing:
Screen output or output files showing the error message and context:
The text was updated successfully, but these errors were encountered: