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

Adding Medlyn stomatal conductance model into CTSM-FATES #608

Merged
merged 17 commits into from
Jun 21, 2020

Conversation

Qianyuxuan
Copy link
Contributor

@Qianyuxuan Qianyuxuan commented Feb 5, 2020

Adding the unified stomatal optimization codes into CTSM-FATES model

Description:

I have adapted the unified stomatal optimization (abbreviated as USO, Medlyn et al. 2011) codes in CLM5 into FATES, including reading two parameters (medlynintercept and medlynslope) from the parameter file and adding a quadratic solution of stomatal conductance. I added the two parameters according to the values in CLM5 based on PFTs. A flag has been created to switch between the Ball-Berry stomatal conductance model and the USO model into FATES. I also removed some hard-coded numbers in existing codes about photosynthesis and stomatal conductance.

Collaborators:

Discussions with @alistairrogers @serbinsh

Expectation of Answer Changes:

Checklist:

  • My change requires a change to the documentation.
  • I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
  • I have read the CONTRIBUTING document.
  • FATES PASS/FAIL regression tests were run
  • If answers were expected to change, evaluation was performed and provided

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

@Qianyuxuan Qianyuxuan changed the title Added Medlyn stomatal conductance model in CTSM_FATES Adding Medlyn stomatal conductance model in CTSM_FATES Feb 5, 2020
@Qianyuxuan Qianyuxuan changed the title Adding Medlyn stomatal conductance model in CTSM_FATES Adding Medlyn stomatal conductance model into CTSM_FATES Feb 5, 2020
@serbinsh
Copy link

serbinsh commented Feb 5, 2020

@rgknox We still need to update the parameter file with this PR to make it consistent with the code changes. I just want to confirm that the appropriate way to do this is to modify the parameter file in the parameter_files directory with the edits and then add that to this pull request, correct?

@rgknox
Copy link
Contributor

rgknox commented Feb 5, 2020

Hi @serbinsh
Ideally we try to bundle changes to the parameter file into groups that are submitted together, having fewer total sets of changes. This helps reduce the burden of folks adapting to changes frequently. We just released a grouped set of parameter changes this January.
I think there a few things we can do here. Lets look around and see if anyone else has updates to the parameter file and see what we have. If nothing else is forthcoming, check in with the team and perhaps just move forward with a change. Or for the short term perhaps we could consider using existing parameters as surrogates for the new ones (ie like bb_slope); until we have a critical mass of parameter updates.

@Qianyuxuan Qianyuxuan changed the title Adding Medlyn stomatal conductance model into CTSM_FATES Adding Medlyn stomatal conductance model into CTSM-FATES Feb 7, 2020
@rgknox
Copy link
Contributor

rgknox commented Feb 19, 2020

Another question, I'm noticing that the Medlyn method, in both FATES and CLM, the conductance is decoupled from leaf water potential. In other words, the Medlyn method does not use the brtan value from what I can tell (am I wrong here?). I know that Medlyn is all about the VPD, but if conductance doesn't respond to LWP, then this seems like it will give the hydraulics module a tough time. My gut is telling me that everyone knows this and its just how it is and I missed the boat, or that I'm missing a term somewhere....

@rgknox
Copy link
Contributor

rgknox commented Feb 19, 2020

nevermind, I forgot that btran scales down vcmax...

@ckoven
Copy link
Contributor

ckoven commented Feb 19, 2020

actually isn't it a valid point, but only as applied to the minimum conductance term? in the b-b scheme, the bbopt gets multiplied by btran, but is that not the case for the medlynintercept term?

@serbinsh
Copy link

serbinsh commented Feb 25, 2020

@rgknox @ckoven I am not sure I am following this discussion. First, the underlying basic Medlyn or Ball Berry leaf stomatal models do not have any BTran or other g0/g1 scalars associated with them. These are bolt-ons that have been added to the model to account for soil water supply (or other stress factors), but I would argue should be considered separately from the basic underlying equations. That is, for clarity sake I think we should be using the core underlying stomatal models as they have been published and only scaling parameters outside of the quadratic before solving. That way we are using the core model and later changes/additions to gs modeling etc that could be impacted by these scaling factors should be able to bypass these, if needed. If they are embedded in the quadratic it might be possible to make a change and not realize that another modifier still exists.

So, as you state currently in FATES bbopt is multiplied by Btran because at some point this was added as a stress scalar. But this is only one way or hypothesis on how this could happen, correct? Because we do this with BB does not mean would should do this with Medlyn, unless we have a reason for this scaling, correct? Also we do not yet have evidence that we should be modifying Medlyn parameters or solved gs by LWP and as such I strongly advocate we do not bake this in yet. Jin's paper, for example, showed that LWP was not a significant factor controlling gs. This again is a hypothesis/research question that I agree we need to explore to identify how best we would incorporate things like Btran or LWP scalars, but which are not part of the underlying Medlyn model. As such if we add the core models we can then explore multipliers that modify the model parameters or results but I dont think we need to add that in now since how we would actually do that is an active area of research.

I am not sure this is as clear as it could be via this thread and perhaps more easily discussed over the phone. We could also look at how other models such as CABLE scale Medlyn in relation to soil/LWP (if at all, I actually dont remember now) and try out different approaches. That said, our goal was to provide a first version of the basic model which we could test with, compare with data, etc and then explore if further edits were needed.

One other quick thing, isn't brtran a soil moisture scalar? Are we thinking we need a similar scalar for LWP or that brtran is a proxy for LWP?

@walkeranthonyp
Copy link

walkeranthonyp commented Feb 25, 2020 via email

@alistairrogers
Copy link

I'm with Anthony on the btran application. It's a separate hypothesis to link soil water content to leaf physiology and should be applied to both or neither in a consistent manner but as Shawn says should not be baked into the stomotal model formulation. It is my understanding that hydraulic approaches should not be using btran so this might be moot in FATES (hydro) anyway. And yes this is a bigger conversation.

We touched on it briefly in Rogers et al (2017) but also see DeKauwe et al (2013, 2015)

The question of whether you make the intercept the same for both models is tricky. When you fit slope and intercept from the same gas exchange dataset you obviously get different slopes but the intercepts are also different (admittedly small numbers anyway). That's why I would advocate for have slope and intercept as PFT specific parameters that are paired. Initially you could set the intercept to be the same for all PFTs and maybe the same with both models but keeping them as paired PFT specific parameters would future proof the model.

@serbinsh
Copy link

Thanks @walkeranthonyp and @alistairrogers Hopefully my comments made sense and didn't come across as anything other than trying to better understand the requests and rationale, but I think you both really expanded and clarified so thank you. I agree we need to be consistent but as stated I was thinking that perhaps before we lock something in we should perhaps explore what approaches/hypotheses we should be using in FATES first, but of course that could also lead us to delay model updates. As such, I am happy to go with the group on this and do what we think is best but also spend time exploring alternatives. As long as we document what we have done well in the code and tech doc it should be easy for us to later address any of these assumptions and make changes without the risk of tuning other processes to something fixed in the model that we then later regret if we try to undo it. That was my main concern.

@walkeranthonyp
Copy link

walkeranthonyp commented Feb 26, 2020

@arogers I hear what you're saying about the intercepts being different. But conceptually it's the same parameter that has the same units, just with a different value. If we had a different parameter for every time a parameter took a different value we'd have an infinite number of parameters.

... also as you pointed out earlier, you're not even estimating g0 with the medlyn model (nor did Lin 2015). A zero intercept is assumed. So conceptually and pragmatically what are we to do with g0? This goes back to an earlier discussion prompted by @Qianyuxuan's addition of the medlyn model.

Should we abandon g0 from the gs calculation in response to environment and use it solely as a minimum value instead? It's unlikely to have a big effects in productive areas during the day time, but at night and in dry areas could be significant. ... a bonus would be that it would simplify the photosynthesis code.

@alistairrogers
Copy link

@walkeranthonyp point taken. My thought was that since estimates of slope and intercept are not independent PFT specific slopes should be paired to PFT specific intercepts. Obviously g0=0 simplifies things considerably and as you point out would have a very low impact (other than arid environments) on daytime gas exchange. If you do go with g0=0 how do we handle the issue of nighttime minimum conductance raised by Danica and described in her GMD paper?

@rgknox
Copy link
Contributor

rgknox commented Feb 26, 2020

I should clarify my earlier point. When Brad and Chonggang's plant hydraulics module is turned on, we calculate a btran value that is based off of LWP. It uses a LWP, P50 and a vulnerability parameter to generate a fraction of total conductance. When that module is off, the btran value is a true btran, and based off of soil moisture conditions. As it currently stands, this fraction of total conductivity value is only applied to vcmax, and bbopt.

@arogers
Copy link

arogers commented Feb 26, 2020 via email

@walkeranthonyp
Copy link

Sorry old chap, my bad - his email is arogers. Any thoughts on keeping g0 as one or multiple parameters?

@walkeranthonyp
Copy link

@walkeranthonyp point taken. My thought was that since estimates of slope and intercept are not independent PFT specific slopes should be paired to PFT specific intercepts. Obviously g0=0 simplifies things considerably and as you point out would have a very low impact (other than arid environments) on daytime gas exchange. If you do go with g0=0 how do we handle the issue of nighttime minimum conductance raised by Danica and described in her GMD paper?

@serbinsh - course, always up for the discussion. The btran question is definitely relevant, even to hydro imo given that there is evidence for signaling between roots and leaves. We should probably have a separate btran thread / discussion, especially as it sounds like transpiration in hydro is pretty low atm.

  • hear you on the non-independence of slope and intercept @alistairrogers , it wants looking out for, but imo it's more efficiently dealt with in the parameter file through proper parameterisation than by adding more parameters to the code.
  • re g0 = 0. I would recommend leaving as is for now and adding that discussion to the btran discussion. If we decide we want to handle g0 as a minimum rather than an intercept we would just need to remove g0 from the quadratic soln. for gs (i.e. derive new solns.). The code to apply g0 as a minimum gs already exists and is in operation.

@rosiealice
Copy link
Contributor

Just noting that as I recall, Polly was interested in looking at how variations in minimum.conductance affected PFT performance in her simulations, hence, I think I'd advocate for it being a PFT level parameter, at least (

@rosiealice rosiealice closed this Feb 27, 2020
@rosiealice rosiealice reopened this Feb 27, 2020
@rosiealice
Copy link
Contributor

...and Ive no idea how I closed the pull request (writing on my phone). Apologies!

@pollybuotte
Copy link

Yes @rosiealice , investigating how minimum conductance affects PFT performance is on my list of "want to dos". It's currently pretty far from the top!

@serbinsh
Copy link

@Qianyuxuan Here is a link to what I mentioned on the param file update thread: #616 (comment). This contains the basic steps needed to complete this PR. Again we will not be using a namelist option for this, just a new param file switch which makes this PR easier

@Qianyuxuan
Copy link
Contributor Author

@serbinsh Thanks! I like the idea of using parameter file switch to choose between the two models. Now I am thinking about how do we deal with the different magnitudes of minimum conductance (g0) in the two models if we only use one parameter. Can we create a scalar in the codes to transfer that? Or do we need to create two parameter files with different values of g0?

@rgknox rgknox self-assigned this Apr 22, 2020
@Qianyuxuan
Copy link
Contributor Author

@rgknox Thanks for your advice! I updated the parameter file again. This time I reverted the parameter file with the original ".cdl" file (fortunately I saved essential files frequently) first and made changes in the ".cdl" file. Then I tested the file by converting the file into the netcdf file and ran the model. The model worked well. I also merged the latest changes in host model and fates. Please feel free to let me know if I need to do more adjustments.

@@ -162,7 +169,7 @@ subroutine FatesPlantRespPhotosynthDrive (nsites, sites,bc_in,bc_out,dtime)
real(r8) :: mm_ko2 ! Michaelis-Menten constant for O2 (Pa)
real(r8) :: co2_cpoint ! CO2 compensation point (Pa)
real(r8) :: btran_eff ! effective transpiration wetness factor (0 to 1)
real(r8) :: bbb ! Ball-Berry minimum leaf conductance (umol H2O/m**2/s)
real(r8) :: stomatal_intercept_btran ! minimum leaf conductance (umol H2O/m**2/s)
Copy link

@serbinsh serbinsh May 14, 2020

Choose a reason for hiding this comment

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

@Qianyuxuan is this the new g0*Bsw? which will go into both stomatal models that we discussed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@serbinsh Yes. "stomatal_intercept_btran" is the water-stressed g0, which will go into both stomatal models.


associate( &
c3psn => EDPftvarcon_inst%c3psn , &
slatop => EDPftvarcon_inst%slatop , & ! specific leaf area at top of canopy,
! projected area basis [m^2/gC]
woody => EDPftvarcon_inst%woody) ! Is vegetation woody or not?
woody => EDPftvarcon_inst%woody , & ! Is vegetation woody or not?
stomatal_intercept => EDPftvarcon_inst%stomatal_intercept ) !stomatal intercept for Ball-Berry model and Medlyn model

Choose a reason for hiding this comment

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

@Qianyuxuan shall we just comment as:

"Intercept or g0 of stomatal conductance model" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@serbinsh okay.

@serbinsh
Copy link

@Qianyuxuan I didnt see an explicit logging of Medlyn stomatal error in the code, just a specific place in the code for logging Ball Berry errors. Should we have something similar for Medlyn? @rgknox ? Or maybe I missed it?

write (fates_log(),*) 'CF: Ball-Berry error check - stomatal conductance error:'
write (fates_log(),*) gs_mol, gs_mol_err
end if
if (abs(gs_mol-gs_mol_err) > 1.e-01_r8 .and. (stomatal_model == 1)) then

Choose a reason for hiding this comment

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

@Qianyuxuan @rgknox this part. It used to be specific to ball berry but maybe this is more general and should instead say something like "Stomatal model error check - stomatal conductance error:" or similar

I assume this is a relevant check for any stomatal model yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I was thinking about it but then hesitated to include it. I found CLM5 didn't code the logging of Medlyn model error. I can definitely include those codes if it is a common procedure.

Choose a reason for hiding this comment

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

@Qianyuxuan OK. @rgknox @glemieux this looks like a basic error/mismatch calculation, any reason to think it cant be generic to both models? or maybe I am missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think your right, I'd have to look more closely, but yup, I think that convergence error is used in both schemes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I will add that to my next commit!


fates_leaf_stomatal_slope_medlyn = 4.1, 2.3, 2.3, 4.1, 4.4, 4.4, 4.7, 4.7,
4.7, 2.2, 5.3, 1.6 ;

Choose a reason for hiding this comment

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

@Qianyuxuan looks good nice work on the updates here. Question: I know you wrote up documentation to that is a companion to your code changes, we should make sure that makes it into the documentation for FATES/tech doc and that also includes these defaults and where they com from for Medlyn (USO). have you shared that documentation you created on this PR yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@serbinsh I haven't yet. I will drop a doc into the comment box here soon.

Copy link

@serbinsh serbinsh left a comment

Choose a reason for hiding this comment

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

@Qianyuxuan I left some comments to address but otherwise its pretty much there I think.

@rgknox
Copy link
Contributor

rgknox commented May 14, 2020

@Qianyuxuan I didnt see an explicit logging of Medlyn stomatal error in the code, just a specific place in the code for logging Ball Berry errors. Should we have something similar for Medlyn? @rgknox ? Or maybe I missed it?

Like here:

if (abs(gs_mol-gs_mol_err) > 1.e-01_r8 .and. (stomatal_model == 1)) then
?

Yeah, if there is a way to quantify solution error and check-it, I think we should do that.

@Qianyuxuan
Copy link
Contributor Author

Here is the corresponding technical note:
Note for Medlyn stomatal conductance model in FATES-LQY-0515.docx


! Compare with Medlyn model: gs_mol = 1.6*(1+m/sqrt(vpd)) * an/leaf_co2_ppress*p + b
if ( stomatal_model == 2 ) then
gs_mol_err = 1.6*(1 + medlyn_slope(ft)/sqrt(vpd))*max(anet,0._r8)/leaf_co2_ppress*can_press + stomatal_intercept_btran
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Qianyuxuan , what is the 1.6, is that the diffusivity ratio of water to carbon?

h2o_co2_bl_diffuse_ratio

Copy link
Contributor

Choose a reason for hiding this comment

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

wait, thats the boundary layer diffusion ratio, I think this is the stoma ratio:
h2o_co2_stoma_diffuse_ratio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rgknox Sorry. I forgot to change that. Yes. 1.6 is the factor that converts from conductance to CO2 to conductance to water vapour. I will adjust it soon.

Copy link

@serbinsh serbinsh left a comment

Choose a reason for hiding this comment

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

Looks good @Qianyuxuan. @walkeranthonyp, over to you for review if you have time.

@serbinsh
Copy link

Here is the corresponding technical note:
Note for Medlyn stomatal conductance model in FATES-LQY-0515.docx

@Qianyuxuan We need to update the tech docs for FATES with this info. Lets take a look at this together.

Copy link

@walkeranthonyp walkeranthonyp left a comment

Choose a reason for hiding this comment

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

@Qianyuxuan great job! Really much easier to understand this code with your edits to the parameter names etc.

@rgknox
Copy link
Contributor

rgknox commented May 20, 2020

@Qianyuxuan , it looks like this PR is in good shape. I don't think anymore action is required on your end. I will check back in after we have updated the master branch to include the parameters.

@Qianyuxuan
Copy link
Contributor Author

@walkeranthonyp @rgknox Sure! Thank you all for your thoughtful comments and continuous supports!

@glemieux
Copy link
Contributor

glemieux commented Jun 8, 2020

Testing in progress.
UPDATE: all expected PASS b4b

/glade/u/home/glemieux/scratch/clmed-tests/pr608-Ce33b4658-Fcc04d703.fates.cheyenne.intel
/glade/u/home/glemieux/scratch/clmed-tests/pr608-Ce33b4658-Fcc04d703.fates.cheyenne.gnu

@rgknox this is good to go

@rgknox rgknox merged commit 9a4627a into NGEET:master Jun 21, 2020
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.

10 participants