search

ESCOMP / CTSM

Community Terrestrial Systems Model (includes the Community Land Model of CESM)
http://www.cesm.ucar.edu/models/cesm2.0/land/
Other
302 stars 307 forks source link

k_soil_root uses incorrect soil conductance #1000

Open djk2120 opened 4 years ago

djk2120 commented 4 years ago

Brief summary of bug

PHS calculates k_soil_root for each soil layer based on hk_l. However, hk_l is defined at the interface between two adjoining soil layers. For PHS the relevant value is not at the interface, but just within the given soil layer.

General bug information

CTSM version you are using: release-clm5.0.30

Does this bug cause significantly incorrect results in the model's science? Yes, but answer changes expected to be relatively small.

Configurations affected: use_hydrstess=.true.

Details of bug

Here the value of hk_l should be replaced with an alternative variable representing hk for a single soil layer: https://github.com/ESCOMP/CTSM/blob/ad9944e57bf420482268d02fe3d08a3966ce23d6/src/biogeophys/PhotosynthesisMod.F90#L2836 Such a variable would need to be calculated within SoilWaterMovementMod. As such it may be appropriate to rename hk_l to hk_interface

As it stands, hk_l effectively averages together the hk values from soil layers i and i+1, whereas PHS needs hk based just on soil layer i. This bug can cause some (minor) incorrect dynamics in the vertical distribution of soil water, but effects on downstream variables like photosynthesis, transpiration, soilwater_10cm, and TWS are all expected to be relatively small.

billsacks commented 4 years ago

@djk2120 thanks for reporting this. Would you be able to take a stab at fixing it and submitting a pull request?

djk2120 commented 4 years ago

Can do, was filing the issue mainly to remind myself to fix this. I think I'll have a few other small PHS fixes that I will consolidate into one pull request.

On Thu, May 7, 2020 at 9:59 AM Bill Sacks notifications@github.com wrote:

@djk2120 https://github.com/djk2120 thanks for reporting this. Would you be able to take a stab at fixing it and submitting a pull request?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CTSM/issues/1000#issuecomment-625343181, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNHR5S2DU3RYO4DZL5XHJLRQLLEZANCNFSM4MT4T2RQ .

billsacks commented 4 years ago

@djk2120 - Sounds good. The one thing to keep in mind about combining things is that we really like to break things down into:

  1. Answer-changing bug fixes that we want to apply even when running with the CLM5.0 physics

  2. Answer changes that we want to apply in a backwards compatible way, via namelist options

  3. Non-answer-changing cleanup

If at all possible, we try to keep those different things in different branches / pull requests, so they can be tested separately (though it's sometimes okay to combine some changes in (2) and (3), and if the changes in class (3) are really tiny, like fixing some comments, then those can be combined with (1)). If in doubt, please feel free to check in with us regarding which category a given change should fall into. The line between (1) and (2) is often subjective.

billsacks commented 2 years ago

@djk2120 what is the status of this?

Update (2022-01-18): Daniel says: nothing new on this; it is a minor bug, but should be fixed at some point.

wwieder commented 1 year ago

@djk2120 this seems like a minor fix that come in with other answer changing tags. Does it make sense to open a PR that fixes the issue? If so, can you do this?