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.

Sign up for GitHub

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

Jump to bottom

Clean up soil vertical layers definition #279

Closed
billsacks opened this issue Feb 13, 2018 · 2 comments · Fixed by #759
Closed

Clean up soil vertical layers definition #279

billsacks opened this issue Feb 13, 2018 · 2 comments · Fixed by #759
Assignees
@slevis-lmwg
Labels
code health improving internal code structure to make easier to maintain (sustainability)

Comments

@billsacks
Copy link
Member

billsacks commented Feb 13, 2018 •
edited
Loading

Moving a comment I made 2015-10-09 to here

I was looking at the new initVerticalMod on the branch that @bandre-ucar is about to bring to the trunk, because it ties in with the initInterp vertical interpolation stuff I'm working on. In doing so, I noticed some duplicated code / logic in setting up the vertical structure: It seems like there are basically two options:

  1. Fundamentally set zsoi, then set dzsoi and zisoi based on that

  2. Fundamentally set dzsoi, then set zsoi and zisoi based on that

However, the code to set zsoi based on dzsoi (or vice versa), and the code to set zisoi based on the others is duplicated for each of the 4(?) options. To me, having this near-duplicated code obscures the real differences between the different methods.

I feel this should look like:

if (method 1) then
   code to set zsoi...
   call set_dzsoi_from_zsoi(dzsoi, zsoi)
else if (method 2) then
   code to set dzsoi...
   call set_zsoi_from_dszoi(zsoi, dzsoi)
else if ...
...
end if

call set_zisoi_from_zsoi(zisoi, zsoi)

See also #728
@billsacks
Copy link
Member Author

Here was the response from @swensosc 2015-10-09

it's not simply an order of operations thing. The original code sets the nodes based on an exponential function, then specifies the interfaces to be midway between the nodes. I've spoken with @martynpclark about this, and I think we agreed that it wasn't the proper way to discretize things. The proper thing to do (I think) is to specify the layer thicknesses, with the assumption that the nodes are at the middle of each layer. Then the interface depths just follow from summing the dz's. Because the two ways of doing things are based on different assumptions about the relationship between the node position in the layer, I would prefer to somehow have that information clear. Going forward, I would like to see method 2 used for alternative soil discretization.

I agree the code could be cleaner. I am picturing it looking a bit different than the above, with submethods:

if (method 1 = "node-based") then
case 1 ("standard")
code to set zsoi...
case 2("more_vertlayers")
code to set zsoi...
call set_dzsoi_and zisoi_from_zsoi(dzsoi, zisoi, zsoi)
else if (method 2 = "thickness-based") then
case 1 ("4-meter, 20 layer")
code to set dzsoi...
case 2 ("8-meter, 20 layer")
code to set dzsoi...
case 3 ("10-meter, 49 layer")
code to set dzsoi...
call set_zisoi_from_dzsoi(zisoi, dzsoi)
call set_zsoi_from_zisoi(zsoi, zisoi)

else if ...
...
end if

@billsacks billsacks added type: enhance - science enhancement new capability or improved behavior of existing capability code health improving internal code structure to make easier to maintain (sustainability) and removed enhancement new capability or improved behavior of existing capability labels Feb 13, 2018
@billsacks billsacks mentioned this issue May 23, 2019
Closed
slevis-lmwg added a commit to slevis-lmwg/ctsm that referenced this issue Jul 6, 2019
@slevis-lmwg
First draft: soil layer definition clean-up and user-defined option
cd05cc7 
1) Clean-up following @swensosc and @billsacks suggestions in issue ESCOMP#279
2) Initial proposal for user-defined option of soil layer structure:
- Use existing namelist variable soil_layerstruct
- Instead of setting to an existing option (e.g. '5SL_3m', etc.),
  set soil_layerstruct = 'user:aa,bb,cc,dd,ee,ff,gg,...'
  where the string is declared of len=256 and
        'user:' tells the code to expect a user defined structure,
        aa gets assigned to nlevsoi and nlevgrnd in the code,
        bb gets assigned to dzsoi(1) in the code,
        cc gets assigned to dzsoi(2) in the code,
        and so on to dzsoi(nlevgrnd).
@slevis-lmwg slevis-lmwg mentioned this issue Jul 6, 2019
Merged
@billsacks
Copy link
Member Author

From discussion: There are still two node-based options. It doesn't add much complexity to the code to keep it in, so we'll do that.

billsacks added a commit that referenced this issue Aug 1, 2019
@billsacks
Merge branch 'soil_layer_def_cleanup'
00769d5 
Soil layer definition clean-up and user-defined option

Code clean-up clarifes that there are two types of soil layer
definition: the node-based and the thickness-based.

User-defined option allows user to specify a soil layer profile in the
form of a dzsoi vector (values in meters) in the thickness-based
approach.

Default nlevsoi for NWP configurations had to change from 5 to 4 for
consistency with the new error check described in known bugs below.

Other code clean-up removes a couple of sections of repeating code.

Resolves #279
Resolves #728
mariuslam pushed a commit to NordicESMhub/ctsm that referenced this issue Aug 26, 2019
@rgknox
Merge pull request ESCOMP#279 from ckoven/sitelevelspread
2c6f697 
Modified crown allometric spread logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@slevis-lmwg slevis-lmwg

Labels
code health improving internal code structure to make easier to maintain (sustainability)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Soil layer definition clean-up and user-defined option slevis-lmwg/ctsm
2 participants
@billsacks @slevis-lmwg