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

Warning statement in create_cohort triggered for every restart #133

Closed
rosiealice opened this issue Oct 10, 2016 · 7 comments
Closed

Warning statement in create_cohort triggered for every restart #133

rosiealice opened this issue Oct 10, 2016 · 7 comments
Assignees

Comments

@rosiealice
Copy link
Contributor

Summary of Issue:

The "write(iulog,*) 'ED: something is zero in create_cohort'" warning statement in (L99 EDCohortDynamicsMod.F90) is supposed to flag when there are either negative or zero values in a new cohort for some reason.

However, at some point during the refactor, a call to 'create_cohort' was inserted which sets most of the cohort state variables to zero (L 2039 of EDRestVectorMod.F90). This means that the warning is triggered for every cohort, which is both wrong and an enormous timesink for writing the cesm.log file...

Potential fixes include:

  1. Getting rid of this altogether (easiest option, loses plausibly useful functionality)
  2. Moving it out of create_cohort into the individual routines where create_cohort is called (adds annoying duplication)
  3. Changing the logic of whatever is happening with the restart files. e..g make the default setting a very small positive number (I don't understand the downsides of that one).

Thoughts?

@rgknox
Copy link
Contributor

rgknox commented Oct 10, 2016

I see it triggers on dbh, n, pft, canopy_trim and balive, all of which are arguments.

In createPatchCohortStructure, where its called, I see we have set balive, and canopy trim to zero (the reason for the message), and given a nominal value of 700 to n, and hard-coded height and dbh and pft.

The whole purpose at this time in the call sequence is to just initialize the cohort and add the memory space onto the linked list. So it shouldn't matter what the argument values are set to...

I like having the check (although I seemed to have ignored it myself :(). I think we should just change the dummy argument values to something non-zero, and perhaps somehting very large that will generate obvious nonsense if the values are not-overwritten with the real information.

lines 2011-2016, change zero's to a global parameter something like:

real(r8),parameter :: fates_fail_large = 9.9e12_r8

@rosiealice
Copy link
Contributor Author

I like the idea of changing it to a nonsense-high number...

On 10 October 2016 at 16:59, Ryan Knox [email protected] wrote:

I see it triggers on dbh, n, pft, canopy_trim and balive, all of which are
arguments.

In createPatchCohortStructure, where its called, I see we have set balive,
and canopy trim to zero (the reason for the message), and given a nominal
value of 700 to n, and hard-coded height and dbh and pft.

The whole purpose at this time in the call sequence is to just initialize
the cohort and add the memory space onto the linked list. So it shouldn't
matter what the argument values are set to...

I like having the check (although I seemed to have ignored it myself :().
I think we should just change the dummy argument values to something
non-zero, and perhaps somehting very large that will generate obvious
nonsense if the values are not-overwritten with the real information.

lines 2011-2016, change zero's to a global parameter something like:

real(r8),parameter :: fates_fail_large = 9.9e12_r8


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#133 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AMWsQzLBWIBvywDQAQD8MmPlDl5uMOVAks5qysNLgaJpZM4KTBKo
.


Dr Rosie A. Fisher

Terrestrial Sciences Section
Climate and Global Dynamics
National Center for Atmospheric Research
1850 Table Mesa Drive
Boulder, Colorado, 80305
USA.
+1 303-497-1706

http://www.cgd.ucar.edu/staff/rfisher/

@rgknox
Copy link
Contributor

rgknox commented Oct 10, 2016

I will put this in my to-do queue. But will wait a little to see what others think.

@rgknox rgknox self-assigned this Oct 10, 2016
@bandre-ucar
Copy link
Contributor

'Warnings' like sound this like a good idea, but are often useless in practice because people rarely look at them and follow up. If there is a real issue with zero values, it should be a runtime error.

I'm fine with setting it to a nonsense number, but we already have several that have been introduced, usually as hard coded constants. We need to create a global parameter, fates_special_value, and, at least for now, set it to the same value that clm uses.

@rosiealice
Copy link
Contributor Author

Fair enough, warnings can indeed go unheeded!

On 10 October 2016 at 20:24, Ben Andre [email protected] wrote:

'Warnings' like sound this like a good idea, but are often useless in
practice because people rarely look at them and follow up. If there is a
real issue with zero values, it should be a runtime error.

I'm fine with setting it to a nonsense number, but we already have several
that have been introduced, usually as hard coded constants. We need to
create a global parameter, fates_special_value, and, at least for now, set
it to the same value that clm uses.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#133 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AMWsQ30hvG8DJTMEeTdArR8Ab0DPepynks5qyvNxgaJpZM4KTBKo
.


Dr Rosie A. Fisher

Terrestrial Sciences Section
Climate and Global Dynamics
National Center for Atmospheric Research
1850 Table Mesa Drive
Boulder, Colorado, 80305
USA.
+1 303-497-1706

http://www.cgd.ucar.edu/staff/rfisher/

@rgknox
Copy link
Contributor

rgknox commented Oct 11, 2016

yes! we do need a global parameter file! Some of these modifications I'm doing on respiration right now are riddled with constants like days_per_year, seconds_per_day, the molar conversion of carbon to kgs, etc. We need FatesConstantsMod.F90

bandre-ucar added a commit that referenced this issue Dec 2, 2016
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
@rgknox
Copy link
Contributor

rgknox commented Dec 6, 2016

close via #147

@rgknox rgknox closed this as completed Dec 6, 2016
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

No branches or pull requests

3 participants