Add a new CoLM option for LSM#656
Add a new CoLM option for LSM#656aureliayang wants to merge 7 commits intoparflow:masterparflow/parflow:masterfrom
Conversation
|
What is the version of the CLM module in Parflow? |
|
@andrewsoong the built-in, subroutine land surface model in ParFlow 3.14 is the Common Land Model (CLM). A predecessor of the Community Land Model. See the introduction of the ParFlow manual and references therein KM08a and DZD+03 as well as the description in section 5.2 in [Kuffour et al.] (https://doi.org/10.5194/gmd-13-1373-2020). |
tikoh-station
left a comment
There was a problem hiding this comment.
Thank you for contributing to ParFlow!
I made a very brief high-level review of your pull-request.
Given the current state of the PR, I am requesting some changes before a more detailed review can be done:
- As pointed out in the code comments, do not reintroduce Issue #588.
- This PR is missing automated CI tests that need to be added with every new feature. Given that this is another land surface model, is has to come with its own comprehensive set of tests.
- This PR is missing documentation for both the input keys and documentation about the model itself. From the example case, it seems like it repurposes CLM keys. That should be stated somewhere in the documentation too.
- Discussion: should CoLM have its own compiler flag?
| if( ${PARFLOW_HAVE_CLM} ) | ||
| add_subdirectory (pfsimulator/clm) | ||
| add_subdirectory (pfsimulator/colm) | ||
| endif( ${PARFLOW_HAVE_CLM} ) | ||
| endif ( ${PARFLOW_HAVE_CLM} ) |
There was a problem hiding this comment.
Should we have different compilation flags for CLM and CoLM?
That way, ParFlow could be compiled with CLM with -DPARFLOW_HAVE_CLM=ON or with CoLM with -DPARFLOW_HAVE_COLM=ON or both by enabling both flags.
| do k = nz, 1, -1 ! PF loop over z | ||
| l = 1+i + (nx+2)*(j) + (nx+2)*(ny+2)*(k) | ||
| if (topo(l) > 0) then | ||
| counter(i,j) = counter(i,j) + 1 | ||
| if (counter(i,j) == 1) then | ||
| numpatch = numpatch + 1 | ||
| topo_mask(1,numpatch) = k | ||
| planar_mask(1,numpatch) = i | ||
| planar_mask(2,numpatch) = j | ||
| planar_mask(3,numpatch) = 1 | ||
| end if | ||
| endif | ||
|
|
||
| if (topo(l) == 0 .and. topo(l+k_incr) > 0) topo_mask(3,numpatch) = k + 1 | ||
|
|
||
| enddo ! k |
There was a problem hiding this comment.
This part of the code would reintroduce Issue #588.
The computation of the bottom index of the domain has been added to ParFlow, so you don't have to recompute it inside CoLM. Check how it was fixed for CLM:
parflow/pfsimulator/clm/clm.F90
Lines 336 to 340 in 515956a
Instead of passing the topo to CoLM, you can just pass the top and bottom index of the domain.
Also, this reintroduction of the issue makes me wonder: how much more overlap is there between the two models CLM and CoLM? At a quick scan, I can't find any more glaring instances where this happens.
Is there a way to group common functionalities?
| #define CoLM_LSM colm_lsm_ | ||
| #endif | ||
|
|
||
| #define CALL_CoLM_LSM(pressure_data, saturation_data, evap_trans_data, mask, porosity_data, \ |
There was a problem hiding this comment.
Issue #588: pass top and bottom index of the domain instead of the mask
| Subvector *p_sub, *s_sub, *et_sub, *m_sub, *po_sub, *dz_sub; | ||
| double *pp, *sp, *et, *ms, *po_dat, *dz_dat; |
There was a problem hiding this comment.
Issue #588: when you pass top and bottom index of the domain instead of the mask, these added variables m_sub and ms are no longer necessary.
Users can use the following key to switch between LSMs
xxxx.Solver.LSM = 'CoLM'
There are two examples in pfsimulator/colm/examples
Once you installed this version parflow
set xxxx.Solver.LSM = 'CoLM' and run the colm_version
set xxxx.Solver.LSM = 'CLM' and run the clm_version