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

variable soil depth compatibility with CLM5 #381

Merged
merged 14 commits into from
Jun 1, 2018

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented May 11, 2018

Description:

THIS IS A BACKWARDS INCOMPATIBLE CHANGE, WILL REQUIRE UPDATES IN HOST. WILL INCREMENT API TAG TO 4.0.0

CLM5 introduced variable soil depths at the column level. Thus, we need to pass these depths into fates at the site level, and modify usage of arrays dictated by this depth accordingly. This also affected the decomposition depth.

Collaborators:

Expectation of Answer Changes:

There may be some round-off differences just because code was moved around, but differences should be small.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the in-code documentation AND 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:

All PASS

@rgknox rgknox self-assigned this May 11, 2018
! These are public contexts that other routines
! should pass as arguments to the generic root profile
! wrapper.
integer, parameter, public :: i_hydro_rootprof_context = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a few comments on what are these parameter refer to will be useful for the understanding of the codes. What is 1 and 2 means too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great, will add

! PROFILE SWAPPING FLAGS. OR IF THERE IS NO DEMAND< LEAVE AS IS.
!
!
! Two context exist 'hydraulic' and 'biomass'
Copy link
Contributor

Choose a reason for hiding this comment

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

a little more explanation what is hydraulic and biomass context represents will be useful

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to incorporate this new root distribution into the hydraulic codes......

@rgknox
Copy link
Contributor Author

rgknox commented May 23, 2018

@xuchongang In the previous version, we had a pre-defined number of rooting layers. nlevsoi_hyd, and it was set to 10 I think by default. Note that now I changed it to nlevsoi_hyd_max.

Question, what is the reasonable range of values that this should take?

I can tell in the code that this allows us to also specify a single root layer, ie nlevsoi_hyd_max = 1.

But, what if there are 20 soil layers? The default with clm5 on the 1x1_brazil compset generated 20 soil layers (above bedrock). Do you still want to specify roots in only the first 10, or would you rather just specify two modes, ie single layer vs however many soil layers exist?

Note on line 406, I'm not sure what to specify here, the arrays are not necessarily the same length.

https://github.com/NGEET/fates/blob/master/biogeophys/FatesPlantHydraulicsMod.F90#L406

I think the two arrays should be indexed by (1:nlevsoi_hyd) (which is the minimum of nlevsoi_hyd_max and the number of soil layers).

EDIT: Regarding line 406.. But for setting z_node_aroot in the trivial case of nlevsoi_hyd=1, does z_node_aroot go to the lowest soil layer?

@rgknox
Copy link
Contributor Author

rgknox commented May 23, 2018

Also, @xuchongang , it looks like we are re-constructing the rooting profile in hydraulics, I should have hydraulics call the new rooting profile function that is used in other parts of the code.

https://github.com/NGEET/fates/blob/master/biogeophys/FatesPlantHydraulicsMod.F90#L317

EDIT: For instance,

Could we add zeng_2001 to the list of root profiles:

https://github.com/rgknox/fates/blob/rgknox-soildepth-clm5/biogeochem/FatesAllometryMod.F90#L1797

And then modify your bisection function to work on the resulting normalized depth profile?

UPDATE: Wait, is the zeng_2001 a depth function, or a function on the rhizosphere radial axis?

@xuchongang
Copy link
Contributor

xuchongang commented May 23, 2018 via email

@xuchongang
Copy link
Contributor

xuchongang commented May 23, 2018 via email

@rgknox
Copy link
Contributor Author

rgknox commented May 23, 2018

ok, the more I look at the code, I can see that there are not so trivial differences between how your hydraulics assesses roots and how the other schemes in fates do it. Lets make an issue and address in a later set of changes.

@rgknox
Copy link
Contributor Author

rgknox commented May 23, 2018

Regarding z_node_aroot.... how about the 1 layer case? Should z_node_aroot be set to the depth of the soil column, or the depth of the first layer?
Where nlevsoil is the number of layers in the soil column dicatated by the host model:

nlevsoil                   = bc_in%nlevsoil
     if ( nlevsoi_hyd == 1) then
        ccohort_hydr%z_node_aroot(1)                = -bc_in%z_sisl(nlevsoil)
     else
        ccohort_hydr%z_node_aroot(1:nlevsoi_hyd)    = -bc_in%z_sisl(1:nlevsoi_hyd)
     end if

Does that look right?

…dr%z_node_aroot in plant hydraulics with new soil depth system
@xuchongang
Copy link
Contributor

Ryan and I had a phone conversaion. For the case nlevsoi_hyd == 1, this is related to the total soil depth that the soil can reach to. The current code is OK but need to further improvement for this case that nlevsoi_hyd == 1.

Copy link
Contributor

@xuchongang xuchongang left a comment

Choose a reason for hiding this comment

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

Ryan and I identify some potential improvement on the hydraulic side: 1) make the root distribution constistent across the code; 2) improve the special case for one soil layers.

…alue, added code that checks to make sure it is larger than number of soil layers.
@rgknox rgknox deleted the rgknox-soildepth-clm5 branch July 31, 2018 23:21
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.

2 participants