-
Notifications
You must be signed in to change notification settings - Fork 92
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
Small number protections to CCH water transfer functions #1164
Conversation
…ns so that it will tolerate incredibly small water contents
This may address: #1154 |
I tried this out -- in the control case, without this PR, the case fails with this error:
In the test case, pulling in the changes to FatesHydroWTFMod.F90, it fails with this error:
In both cases I'm using this E3SM branch with this FATES branch (with the change to FatesHydroWTFMod for the test case). I can try also testing with this PR and the hydro stability PR. The region used in these runs is a lat/lon rectangle from 15 N - 25 S and 30 W - 85 W so it includes very dry regions outside the Amazon basin where FATES-Hydro is unhappy. |
Running regression tests on derecho I found that while this resolves #1163, I'm seeing a run failure due to the 1D hydro solver failing with a large
I'm guessing that this is due to something causing Note that the log stops writing early due to #1168, but I'm not entirely sure if that error is actually due to the above error. |
Unfortunately I also get this error with a domain restricted to the Amazon Basin and when including @xuchongang's hydro stability branch. Also, I forgot to mention that I am using fates_hydro_solver = 2 . |
I should note that the regression tests for this were all b4b otherwise. derecho location: |
biogeophys/FatesHydroWTFMod.F90
Outdated
th = this%th_sat*(psi/this%psi_sat)**(-1.0_r8/this%beta) | ||
else | ||
if(psi<this%psi_min) then | ||
th = this%th_min |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this artificially add water to soils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but the intention is to go from a value of zero to an inconsequentially small non-zero value. @ckoven and I were talking, this could potentially be problematic if we artificially add small amounts of water and that enables or drives non-zero fluxes that would had otherwise been zero. One possibility would be to force fractional conductivity to zero where it hits theta min. That seems messy though. Honestly, I think if we just use a suitably small minimum value, one that is an incredibly small water content, yet can still accomodate the math needed to switch between psi and theta, this should be sufficient. Perhaps we can try minimum capping at -15 MPa or smaller, perhaps -9 is too high.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this trigger mass balance check error if we run the model for a long time (let's say 1000 years for spinup) ? Should we should keep track of this error and used it as part of the mass balance error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is a mass conservation liability. We use these functions to drive fluxes, they never actually over-write the original masses of each pool. It is the fluxes that update the pools. What will happen is that water contents below the psi_min or theta_min values will just end up having zero flux out of that pool, and thus remain unchanged.
For instance here is where we update the actual soil water content, it is based off of fluxes:
https://github.com/NGEET/fates/blob/sci.1.74.0_api.35.0.0/biogeophys/FatesPlantHydraulicsMod.F90#L2729-L2730
…defined variables, reduce minimum suction of cch and fixed an error check
Bug fix. Adrianna Foster identified that trunks should be removed from the patch-level variable sum_fuel before being used to calculate rate of spread and fuel consumption.
…luxes to soil layer fluxes when no conductance (trivial bug fix).
These changes seem to be adding some stability. I ran a 25 year smoke test on the F45 grid with ELM-FATES-Hydro. Vegetation was initialized as a cold start using the specified stem diameter (which I specified as the diameter at 75% maximum height) and closed canopy assumption, along with fixed biogeography and no-competition assumptions. All parameters were defaults, with the one exception being the use of the Picard hydraulics solver. The model completed the smoke test. I show some mean fluxes and states from the final year of simulation. I interpret from the output that the hydraulics model continued to generate fluxes within the realm of realism through the final year of simulation. Note that contrary to the Picard solver, the 1D Taylor solve (default) did crash after several years, having trouble generating a solution within the error tolerance. |
I think this PR is ready for another round of review, and has changed significantly since the first round. I plan no further changes beyond what comes out of review. |
Wow, this is great @rgknox ! When perlmutter comes back up I'll try rerunning my various failed hydro cases with this branch and hopefully close several of my hydro Issues. |
I'm in favor of making Picard the default. I'm curious if others have found similar things. I'll create a discussion! |
This PR with fates_hydro_solver=2 allows the cases to run successfully. Like you found, with fates_hydro_solver=1, we still get "Could not find a stable solution for hydro 1D solve" errors in dry gridcells |
Chiming in here.
This is fantastic news and sounds to me like the Picard solver should be
the default.
…On Mon, Apr 22, 2024, 10:33 AM Jennifer Kowalczyk (Jenny) < ***@***.***> wrote:
Wow, this is great @rgknox <https://github.com/rgknox> ! When perlmutter
comes back up I'll try rerunning my various failed hydro cases with this
branch and hopefully close several of my hydro Issues. Should we make the
Picard 2D solver the default?
This PR with fates_hydro_solver=2 allows the cases to run successfully.
Like you found, with fates_hydro_solver=1, we still get "Could not find a
stable solution for hydro 1D solve" errors in dry gridcells
—
Reply to this email directly, view it on GitHub
<#1164 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADSXQZRRO2RVUUSOQBUKPTTY6UUUFAVCNFSM6AAAAABDVVGDSCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRZHEYTGMBXG4>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
biogeophys/FatesHydroWTFMod.F90
Outdated
@@ -44,7 +50,8 @@ module FatesHydroWTFMod | |||
! elastic-caviation region | |||
|
|||
|
|||
real(r8), parameter :: min_theta_cch = 0.01_r8 ! Minimum theta (matches ctsm) | |||
!real(r8), parameter :: min_theta_cch = 0.05_r8 ! Minimum theta (matches ctsm) | |||
real(r8), parameter :: min_psi_cch = -20.0_r8 ! Minimum theta (matches ctsm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update comment to say 'Minimum psi'
biogeophys/FatesHydroWTFMod.F90
Outdated
!this%th_min = min_theta_cch | ||
!this%psi_min = this%psi_from_th(min_theta_cch+tiny(this%th_max)) | ||
this%psi_min = min_psi_cch | ||
this%th_min = this%th_from_psi(min_psi_cch+tiny(this%th_max)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of the tiny(this$th_max)? Wondering if this is an error, since the input to this function should be only water potential.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originally, this tiny() function was used to prevent a problem with order of operations. Inside these functions we have "if statements" that compare theta and psi to these very psi_min and th_min thresholds we are calculating. By adding or subtracting a tiny, we were preventing the intering of logical blocks that were not yet initialized. But with these changes, we actually don't really use these tiny's as much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understand, but does seems to be a little confusing. Maybe we can clean up the codes in later pull requests.
real(r8) :: pc | ||
real(r8) :: kr | ||
real(r8) :: dkr_dP | ||
real(r8) :: sat_res | ||
real(r8) :: alpha | ||
real(r8) :: lambda | ||
real(r8) :: Se | ||
real(r8) :: deltaPc | ||
real(r8) :: dSe_dpc | ||
real(r8) :: dkr_dSe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment. My memory is a bit fuzzy on my Brooks-Corey model without looking it up; can comments on these variables be made to ease interpretability? And an overall comment citing a companion source for implementing the three main different regimes (full, smoothing, saturated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would be great to add comments on the units and meaning of each variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an issue here: #1193
I would just add the changes in here, but I'm not familiar with that model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished my review. It looks great to me (on minimum water content and forced zero conductance) and I think it should resolve the issue of very dry conditions.
biogeophys/FatesHydroWTFMod.F90
Outdated
@@ -44,6 +44,8 @@ module FatesHydroWTFMod | |||
! elastic-caviation region | |||
|
|||
|
|||
real(r8), parameter :: min_psi_cch = -9._r8 ! Minimum psi we are willing to track in cch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you put the unit here (Mpa)? Why 9 instead of 15?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was actually updated, seems like a github glitch
biogeophys/FatesHydroWTFMod.F90
Outdated
!this%th_min = min_theta_cch | ||
!this%psi_min = this%psi_from_th(min_theta_cch+tiny(this%th_max)) | ||
this%psi_min = min_psi_cch | ||
this%th_min = this%th_from_psi(min_psi_cch+tiny(this%th_max)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understand, but does seems to be a little confusing. Maybe we can clean up the codes in later pull requests.
biogeophys/FatesHydroWTFMod.F90
Outdated
th = this%th_sat*(psi/this%psi_sat)**(-1.0_r8/this%beta) | ||
else | ||
if(psi<this%psi_min) then | ||
th = this%th_min |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this trigger mass balance check error if we run the model for a long time (let's say 1000 years for spinup) ? Should we should keep track of this error and used it as part of the mass balance error?
biogeophys/FatesHydroWTFMod.F90
Outdated
@@ -44,6 +50,9 @@ module FatesHydroWTFMod | |||
! elastic-caviation region | |||
|
|||
|
|||
!real(r8), parameter :: min_theta_cch = 0.05_r8 ! Minimum theta (matches ctsm) | |||
real(r8), parameter :: min_psi_cch = -15.0_r8 ! Minimum theta (matches ctsm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should be minimum water potential (MPa)
|
||
! If the difference between psi and psi-min is greater than 10MPa | ||
! just assume there is no effect of the minimum function (ie weight 0) | ||
min_ftc_weight = exp(min_ftc_scalar*max(psi_min-psi,-10._r8)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is min_ftc_scalar this a parameter value read from parameter file that can be user-defined? can we put 10 MPa as a parameter for future testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When psi gets to about 2 MPA from psi_min, the weighting factor gets incredibly small, the 10 MPa here is just to avoid numerical problems with that exponent function. We could test the min_ftc_scalar though.. I don't expect the choice of min_ftc_scalar to have any noticable impact on model output, but I could try a few different values to see if anything happens. We have the utility parameter fates_dev_arbitrary that could aid in this.
! low suction | ||
call get_min_ftc_weight(psi_min,psi,min_ftc_weight,dmin_ftc_weight_dpsi) | ||
|
||
ftc = ftc*(1._r8 - min_ftc_weight) + min_ftc*min_ftc_weight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this implementation and I think it should resolve a good chuck of issues of overshooting
else | ||
dpsidth = -this%beta*this%psi_sat/this%th_sat * (th/this%th_sat)**(-this%beta-1._r8) | ||
if(th<this%th_min) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to check on the edge cases
real(r8) :: pc | ||
real(r8) :: kr | ||
real(r8) :: dkr_dP | ||
real(r8) :: sat_res | ||
real(r8) :: alpha | ||
real(r8) :: lambda | ||
real(r8) :: Se | ||
real(r8) :: deltaPc | ||
real(r8) :: dSe_dpc | ||
real(r8) :: dkr_dSe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would be great to add comments on the units and meaning of each variables
|
||
return(om_watsat,om_sucsat,om_bsw) | ||
|
||
def CCHParmsCosby84T5(zsoi,om_frac,sand_frac,clay_frac): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put some citation would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
@@ -2767,6 +2784,20 @@ subroutine hydraulics_bc ( nsites, sites, bc_in, bc_out, dtime) | |||
end do | |||
|
|||
! Second pass, apply normalized weighting factors for fluxes | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch for low water content. If the sum of weight is close to zero, then the layer-specific water uptake is proportion of the root x water content here. It might be better to explain more on the new disaggregation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the comment a little, should be more accurate. Does this look good @xuchongang ?
…ro, to enable first time-step issues in ctsm
tests ok, tests_0501-150922de |
A cap has been created on the smallest suction that we are willing to simulate with CCH (Campbell Clapp-Hornberger) water transfer functions. These methods determine the equivalent water content, so that if values below this water content are passed to the functions, it operates at the lowest threshold instead of breaking. The current threshold is -15 MPa. This is incredibly small, and should generate conductances that indistinguishable from conductances generated with near zero water contents.
A further modification was made in a second round of development here. All water retention curves are now designed to have a minimum value. For CCH it is -15 MPa, for Van Genucten it is the residual water content (existing parameter). We now force the fraction of maximum conductivity to be zero at the minimum value, this currently includes stomatal conductance. This is applied by determining a weighting function that blends in a zero value. The weighting is 1 at the minimum suction, and decreases to zero on an exponential. I used an exponent that attenuates to a value of about 0.175 after increasing 1 MPa above minimum, and 0.025 after 2 MPa above minimum. This function makes the derivative of conductance with suction continuous and smooth, but most importantly, it prevents the flow of water in plant and soil media that are completely parched.
This method of ensuring fluxes are zero at these super low water contents will not affect re-hydration, because conductance between nodes is driven by the suction of the up-stream compartment (ie the one with more total potential, or less suction). However, this will prevent completely desicated compartments from going into negative water content.
Description:
Collaborators:
@olyson @jennykowalcz
Expectation of Answer Changes:
This should prevent crashes, particularly in soil water calls using frozen soils.
Fixes: #1168
Fixes: #1163
Checklist
If this is your first time contributing, please read the CONTRIBUTING document.
All checklist items must be checked to enable merging this pull request:
Contributor
Integrator
Documentation
Test Results:
CTSM (or) E3SM (specify which) test hash-tag:
CTSM (or) E3SM (specify which) baseline hash-tag:
FATES baseline hash-tag:
Test Output: