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

fixes to hydraulics #525

Merged
merged 17 commits into from
May 30, 2019
Merged

fixes to hydraulics #525

merged 17 commits into from
May 30, 2019

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Apr 25, 2019

Description:

This changeset includes some important fixes to hydaulics. These fixes were implemented by @xuchongang .
(Since these are high-priority fixes for the hydro team, and @xuchongang is traveling, @rgknox is acting as submitter.)

  1. initialize the plant with more water (not in equilibrium with very dry soil);
  2. reduce conductance for soil-root interface for water loss compared to water uptake;
  3. greatly increased the hard-wired cap on maximum stomatal resistance (e.g. decreases minimum conductance)

Collaborators:

@pollybuotte analyzed, tested and identified errors, @rgknox and @JunyanDing identified errors and provided support.

Expectation of Answer Changes:

The change in maximum resistance should change ALL fates simulations.

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:

TBD

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

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

FATES baseline hash-tag:

Test Output:

@rgknox
Copy link
Contributor Author

rgknox commented Apr 25, 2019

It seems we have two slightly different ways of calculating minimum leaf conductance "bbb" in this routine.

Here we prevent it from going lower than 1.0 umol H2O/m**2/s

bbb = max (bbbopt(nint(c3psn(ft)))*currentCohort%co_hydr%btran(1), 1._r8)

But in these calculations which are only active at night when btran>0, there is no lower bound:

rstoma_out = min(rsmax0, cf*1._r8/(bbbopt(c3c4_path_index)*btran))

My take is that 1 umol H20/m**2/s is pretty small right? The defualt cuticular conductances in our parameter file are 10,000 and 40,000. Is there any reason why we aren't using the BBB calculated in line 400, that is passed as an argument, not to be used in line 972?

@xuchongang @pollybuotte @JunyanDing

@pollybuotte
Copy link

@rgknox I have not gone through the code yet, but given Chonggang's description of his changes...

"There are also other place where the minimum conductance is capped at 1.0 umol/m2/s, mainly for reasonable photosynthesis; but this can also cause a higher stomata conductance for water loss. I have also changed those to be determined by the cuticular conductance inputs and btran."

I would have expected all the places where bbb is calculated to now be based on btran and cuticular conductance.

Copy link

@pollybuotte pollybuotte left a comment

Choose a reason for hiding this comment

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

I do not see any problems with these changes, and they include everything we have discussed this week. I do have several questions, but we can discuss these offline.

@@ -968,6 +983,7 @@ subroutine LeafLayerPhotosynthesis(f_sun_lsl, & ! in

!is there leaf area? - (NV can be larger than 0 with only stem area if deciduous)
if ( laisun_lsl + laisha_lsl > 0._r8 ) then
if(bbb > 1.0_r8)then !only if stomata open larger than cuticular conductance
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xuchongang , I don't follow the connection between the logic and its comment. Can you help me clear this up?

The code is seeing if the minimum leaf (cuticular) conductance (bbb) is greater than 1. But the comment implies that its comparing the stomata opening against cuticular conductance....

Since 1 umol/m2/s is very very small, it seems that this comparison is really about seeing if btran>0 ? I must be missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xuchongang and I checked in on this, and agreed that we can remove this logic: "I basically try to avoid the cap by 1 umol/m2/s and avoid the cases of unstable solutions of gs. I agree that 1 umol/m2/s is very very small and this part of change is not critical for the bug fix. I understand your confusion and I think we can leave the code as it is to avoid this confusion."

@rgknox rgknox requested a review from JunyanDing April 25, 2019 23:19
@rgknox
Copy link
Contributor Author

rgknox commented Apr 26, 2019

All, please see the following additional changes I'm planning to add:
rgknox#2

@rgknox rgknox added PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. and removed High Priority labels Apr 26, 2019
@rgknox
Copy link
Contributor Author

rgknox commented Apr 26, 2019

We have identified that there are still non-trivial mass balance errors generated during the 1D-solve portion of hydraulics. I re-labeled this as "not-ready", and we will continue to evaluate this more after the DOE PI meeting.

Copy link
Contributor

@JunyanDing JunyanDing left a comment

Choose a reason for hiding this comment

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

I just read through the code. It appears to me, in FatesPlantRespPhotosynthMod.F90 most place the minimum conductance is capped are from rsmax0. Since rsmax0 has been increased to rsmax0 = 2.e8_r8, do we still need to remove the caps?
I didn’t get the logic of this newly added block of code line 971 – 975):
if(btran>0._r8) then
rstoma_out = min(rsmax0, cf*1._r8/(bbbopt(c3c4_path_index)*btran))
else
rstoma_out = rsmax0
endif

My understanding is when 0<btran<rsmax0bbbopt/cf, rstoma_out = cf1._r8/(bbbopt(c3c4_path_index)*btran, so rstoma_out decrease with btran and approaches 0, but when btran hits 0, rstoma_out will =rsmax0, so there is a sudden increase of rstoma_out from 0 to rsmax0. I think this will give weird behavior of the model.
Please correct me, if my understanding is wrong.

All the changes in FatesPlantHydraulicsMod.F90 seem good.

@rgknox
Copy link
Contributor Author

rgknox commented Apr 26, 2019

@JunyanDing , can you take a look at the changes I'm staging to be added in rgknox#2. Do these changes look like they will fix this inconsistency?

@rgknox rgknox removed PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. PR status: Needs Review labels May 16, 2019
@rgknox
Copy link
Contributor Author

rgknox commented May 30, 2019

35 year comparison at BCI (the changes to rsmax0 made this non b4b change):

https://drive.google.com/file/d/13A5UJilQ4oP1awBPjrRDnx5o4xl9MAy2/view?usp=sharing

50 year f10 smoke test:

/gpfs/fs1/scratch/rgknox/fates-clm-tests/hydro-fixes-f10_f10_mg37/run

Regression tests (not b4b, should not be):

/gpfs/fs1/scratch/rgknox/clmed-tests/hydro-fix-C6767148-F5dbff93.fates.cheyenne.intel

@rgknox rgknox merged commit 552e9de into NGEET:master May 30, 2019
@rgknox rgknox deleted the xuchongang/HYDRO_UPDATE branch October 31, 2023 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants