ESCOMP / ESCOMP/CTSM

Clean up use_bedrock implementation

Open
#289 7 comments 0 reactions 0 assignees View on GitHub
code health
Dominant language
Fortran
Stars
352
Forks
361
Avg merge
6d 6h
Merged PRs (30d)
8

Description

**These comments were originally made October, 2015. I'm moving them over as is, though I'm not sure if they all still apply.**

**Here was my original comment.**

From my email:

In trying to make sure that I'm classifying layers correctly, I looked through the code for what effect use_bedrock has. I think I'm doing the correct layer classification, but this led me to find some other confusing things:

According to the documentation:

If TRUE, use variable soil depth.

If present on surface dataset, use depth to bedrock information to
specify spatially variable soil thickness. If not present, use bottom
of soil column (nlevsoi).

So I expected to find that use_bedrock simply affected whether the model uses a spatially-variable or a spatially-uniform depth to bedrock. As such, the one place where I expected to see use_bedrock appear was here in initVerticalMod:

if (.not. use_bedrock) then
if (masterproc) write(iulog,*) 'not using use_bedrock!!'
zbedrock_in(:) = zisoi(nlevsoi)
endif

However, I found:

(1) use_bedrock seems to also have some effects in SoilBiogeochemVerticalProfileMod.F90. Is this a documentation error (i.e., really use_bedrock does more than just determine whether you're using spatially-uniform or spatially-variable depth to bedrock)? Or should these conditionals be changed to something else?

(2) use_bedrock also appears in WaterStateType:

if (use_bedrock) then
nbedrock = col%nbedrock(c)
else
nbedrock = nlevsoi
endif

I believe this is unnecessary, because my read of the earlier code is that, if use_bedrock is false, then col%nbedrock will already be set up so that it is equal to nlevsoi everywhere, right?

(This falls under my general desire to only have conditionals appear where they are truly required, so that it's more clear what fundamentally changes in the code when you flip a given switch.)

(3) It is unclear to me why use_bedrock should require bc_zero_flux (in SoilWaterMovementMod): If use_bedrock just relates to whether depth-to-bedrock is spatially-varying, why should it come with any requirement on the boundary conditions? If this is truly a requirement, can you add some comments saying why?

**Here is a checklist of things to do, partly based on that comment, and partly based on other comments, below.**

- [ ] Get rid of conditional mentioned in point (2) of my original comment

- [ ] Add better documentation about what use_bedrock does

- [ ] Use nbedrock rather than nlevsoi in the lake code, then remove note about this inconsistency (see comments elsewhere in this issue)

- [ ] Fix inconsistency in use of nbedrock for wetland columns

- [ ] Initialize soil water consistently in shallow and deep soil layers, if possible (see comment originally made 10-19-15, below)

- [ ] Assuming shallow and deep soil layers can be made consistent in terms of their soil water: Move to having just two classes of layers (see comment originally made 10-19-15, below)

- [ ] Handle grid cells with small values of zbedrock properly - see comment originally made 10-28-15, below

- [ ] Possible threading issue in SoilWaterMovementMod.F90::BaseflowSink() because of whole array assignment.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.