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

bb_slope is read from parameter file, but overwritten prior to use #73

Closed
rgknox opened this issue Jun 7, 2016 · 8 comments · Fixed by #76
Closed

bb_slope is read from parameter file, but overwritten prior to use #73

rgknox opened this issue Jun 7, 2016 · 8 comments · Fixed by #76

Comments

@rgknox
Copy link
Contributor

rgknox commented Jun 7, 2016

Summary of Issue:

bb_slope (stomatal slope, as per BBF photosynthesis lore) is read from the parameter file, indexed by PFT. This parameter is supposedly used by EDPhotosynthesis. The parameter file value is stored in bb_slope, and it is transferred to mbb. bb_slope is used no-where else in the code.

Around line 360:

            ! Soil water stress applied to Ball-Berry parameters
            do FT = 1,numpft_ed
               if (nint(c3psn(FT)) == 1)then
                  ps = 1
               else
                  ps = 2
               end if
               bbb(FT) = max (bbbopt(ps)*currentPatch%btran_ft(FT), 1._r8)

               mbb(FT) = bb_slope(ft) ! mbbopt(ps)
            end do

So bb_slope is used to populate mbb. However mbb is then over-written in the next patch loop, and had not been used prior to the over-write. At roughly 404 of EDPhotosynthesis:

      do f = 1,fn
         p = filterp(f)
         c = patch%column(p)
         s = hsites(c)

         if (patch%is_veg(p)) then

            currentPatch => map_clmpatch_to_edpatch(sites(s), p) 

            do FT = 1,numpft_ed
               if (nint(c3psn(FT)) == 1)then
                  ps = 1
               else
                  ps = 2
               end if
               bbb(FT) = max (bbbopt(ps)*currentPatch%btran_ft(FT), 1._r8)
               mbb(FT) = mbbopt(ps)           # FIX POINT

               if (nint(c3psn(FT)) == 1)then
                  ci(:,FT,:) = 0.7_r8 * cair(p)
               else
                  ci(:,FT,:) = 0.4_r8 * cair(p)
               end if
            enddo

Where you can see above, that the actual parameters being used are two values from Gordon Bonan's paper. Not that those values aren't reasonable, but we should decide where we want the values set.

As far as I can tell, these are values for C3 and C4 pathways.

The two values are 9 (C3 I think) and 4 (C4), the paper is Bonan et al. 2011, JGR, 116, doi: 10.1029/2010JG001593

I think what happened is that when bb_slope was added to the code as a user controlled parameter, the coder just didn't see that the mbb variable was set twice. It actually isn't used in the first appearance.

My recommendation is that we simply remove the first call to mbb and bb_slope, and change the line I flagged "FIX POINT" to:

mbb(FT) = bb_slope(ft)

Have discussed this issue with @xuchongang

What is the changeset ID of the code, and the machine you are using:

all to current

@ckoven
Copy link
Contributor

ckoven commented Jun 7, 2016

would explain the surprising lack of a bb_slope effect in Elias and @xuchongang's sensitivity study?

@rgknox
Copy link
Contributor Author

rgknox commented Jun 7, 2016

yeah, I stumbled across some old notes I had on this, and combined with their results, prompted me to revisit the code

@rosiealice
Copy link
Contributor

Yeah, this was just re-engineered in CLM5. You're correct on all counts. It
should come from the file...
On Jun 7, 2016 18:11, "Ryan Knox" [email protected] wrote:

yeah, I stumbled across some old notes I had on this, and combined with
their results, prompted me to revisit the code


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#73 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AMWsQ6KWNCewDQ1Dnkw9jv961sI5BMzyks5qJfqMgaJpZM4IwcWk
.

@rgknox
Copy link
Contributor Author

rgknox commented Jun 8, 2016

ok, its a simple fix, sounds like we are all in agreement

@rgknox
Copy link
Contributor Author

rgknox commented Jun 14, 2016

There is another parameter that is somewhat coupled in with the slope parameter:

Ball-Berry minimum leaf conductance, unstressed (umol H2O/m**2/s).

Is this cuticular?

We are using some two values hard-coded for C3 and C4 plants. Is there interest in adding these as PFT parameters to the parameter file?

@rosiealice , @xuchongang , others?

@xuchongang
Copy link
Contributor

Ryan,
Thank you very much for identify this issue and suggest the correction.
Yes, it would be great if we could have the cuticular conductance as an
input parameter as it is important for our plant hydraulic models. We have
measurement data on that.

Yours
Chonggang

On Tue, Jun 14, 2016 at 2:14 PM, Ryan Knox [email protected] wrote:

There is another parameter that is somewhat coupled in with the slope
parameter:

Ball-Berry minimum leaf conductance, unstressed (umol H2O/m**2/s).

Is this cuticular?

We are using some two values hard-coded for C3 and C4 plants. Is there
interest in adding these as PFT parameters to the parameter file?

@rosiealice https://github.com/rosiealice , @xuchongang
https://github.com/xuchongang , others?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#73 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AH2Yx1ohd4adYH-yRd0tQMDNI3N9w05_ks5qLwuXgaJpZM4IwcWk
.

@rosiealice
Copy link
Contributor

Actually, this would be great. Danica has a paper on the impact of minimum
stomatal conductance in GMDD at the moment, based evidence it's larger that
it's larger than these hardwired numbers...

On 14 June 2016 at 15:24, chonggang xu [email protected] wrote:

Ryan,
Thank you very much for identify this issue and suggest the correction.
Yes, it would be great if we could have the cuticular conductance as an
input parameter as it is important for our plant hydraulic models. We have
measurement data on that.

Yours
Chonggang

On Tue, Jun 14, 2016 at 2:14 PM, Ryan Knox [email protected]
wrote:

There is another parameter that is somewhat coupled in with the slope
parameter:

Ball-Berry minimum leaf conductance, unstressed (umol H2O/m**2/s).

Is this cuticular?

We are using some two values hard-coded for C3 and C4 plants. Is there
interest in adding these as PFT parameters to the parameter file?

@rosiealice https://github.com/rosiealice , @xuchongang
https://github.com/xuchongang , others?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#73 (comment), or
mute
the thread
<
https://github.com/notifications/unsubscribe/AH2Yx1ohd4adYH-yRd0tQMDNI3N9w05_ks5qLwuXgaJpZM4IwcWk

.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#73 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AMWsQ90JIBScx5m9JSE0kujb3O1IXZ7gks5qLw3ugaJpZM4IwcWk
.


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 Author

rgknox commented Jun 14, 2016

applied corrections, PR to come, testing

bandre-ucar added a commit that referenced this issue Jun 27, 2016
Merge branch 'rgknox-bbfix-careafix-icefiltfix'

This PR bundles 3 fixes that address: #73, #69, #44

The fix to 73 is the only one that would be expected to have b4b
regressions. I performed baseline simulation comparsisons between
f1a14d6 and 18613d1, and tests confirmed b4b on all expected
passes. One extra step was necessary, in that I needed to update the
parameter file values of BB_slope to match what was previously hard
coded (a value of 9). The current value in the default parameter file
is 8, we can certaintly change this going forward. Not changing this
now.

The other two fixes, #69 and #44 are not supposed to generate b4b
results, and they don't. 1x1 brazil simulations were also run on eddi
to make sure that the non-b4b changes continue to generate very
similar projections of forest composition and structure, as well as
flux variables. They did.

Fixes: #73
Fixes: #69
Fixes: #44

User interface changes?: no

Code review: rgknox

Test suite: ed - lawrencium-lr3 intel, eddi (PC) gnu (visualizations);
ed - yellowstone gnu, intel, pgi

Test baseline: 18613d1
Test namelist changes: none
Test answer changes: yes, see above

Test summary: pass except for #14 known failures in f09 and f19
restart, and gnu f10 restart #43.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants