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

Add the soil erosion model #3000

Merged
merged 8 commits into from
Jul 31, 2019
Merged

Conversation

tanzeli1982
Copy link
Contributor

@tanzeli1982 tanzeli1982 commented Jun 16, 2019

Add the soil erosion model to ELM. The changes include the implementation
of soil erosion and erosion induced sediment, C, N and P fluxes from land to rivers.

[BFB]
[NML]

@tanzeli1982 tanzeli1982 added this to the v2.0alpha milestone Jun 16, 2019
@tanzeli1982 tanzeli1982 changed the title Tanzeli/lnd/elm erosion master Add the soil erosion model Jun 17, 2019
@tanzeli1982
Copy link
Contributor Author

tanzeli1982 commented Jun 17, 2019

It is a BFB and NML PR. The code review page for this task can be found here https://acme-climate.atlassian.net/wiki/spaces/ED/pages/976486494/B1+Soil+Erosion.

@rljacob rljacob added the River label Jun 17, 2019
@bishtgautam
Copy link
Contributor

@tanzeli1982, The output of the command shows your PR is BFB and please update the first comment of this PR to indicate it is BFB. (see https://acme-climate.atlassian.net/wiki/spaces/ED/pages/16253965/Commit+and+PR+message+template). Also, update the first comment to include the link the code review page as requested by @rljacob.

@E3SM-Project E3SM-Project deleted a comment from rljacob Jun 17, 2019
@E3SM-Project E3SM-Project deleted a comment from rljacob Jun 17, 2019
@E3SM-Project E3SM-Project deleted a comment from bishtgautam Jun 17, 2019
@E3SM-Project E3SM-Project deleted a comment from bishtgautam Jun 17, 2019
@tanzeli1982 tanzeli1982 added BFB PR leaves answers BFB NML labels Jun 17, 2019
@bishtgautam
Copy link
Contributor

@tanzeli1982, If a question in a comment has been answered, don't delete the comment. Let the comment remain, so someone at a later date can follow the conversation.

components/clm/src/biogeochem/ErosionMod.F90 Outdated Show resolved Hide resolved
components/clm/src/biogeochem/ErosionMod.F90 Outdated Show resolved Hide resolved
components/clm/src/biogeochem/ErosionMod.F90 Outdated Show resolved Hide resolved
components/clm/src/biogeochem/ErosionMod.F90 Outdated Show resolved Hide resolved
components/clm/src/biogeochem/ErosionMod.F90 Outdated Show resolved Hide resolved
components/clm/src/data_types/ColumnDataType.F90 Outdated Show resolved Hide resolved
components/clm/src/data_types/ColumnDataType.F90 Outdated Show resolved Hide resolved
components/clm/src/data_types/ColumnDataType.F90 Outdated Show resolved Hide resolved
components/clm/src/data_types/ColumnDataType.F90 Outdated Show resolved Hide resolved
components/clm/src/main/pftvarcon.F90 Outdated Show resolved Hide resolved
@tanzeli1982
Copy link
Contributor Author

@qzhu-lbl said

One comment is about ColPBalanceCheck 665
primary P and occluded P are not accounted, in
col_poutputs, since they are not tracked in the P balance calculation.

@tanzeli1982
Copy link
Contributor Author

@bishtgautam I have copied three raw data files (mksrf_gravel_10level_5min.c190603.nc, mksrf_slope_10p_0.5x0.5.c190603.nc and mksrf_soilero_0.5x0.5.c190603.nc) to /global/project/projectdirs/acme/inputdata/lnd/clm2/rawdata.

@bishtgautam
Copy link
Contributor

@tanzeli1982, The data has been now added to E3SM inputdata server and is available at https://web.lcrc.anl.gov/public/e3sm/inputdata/lnd/clm2/rawdata/

@thorntonpe
Copy link
Contributor

@tanzeli1982 I'm beginning my review of this PR. The design document indicates that this capability is added for the ECA bgc model. Is it also added for CTC? Since CTC is the default soil bgc formulation, it should also be the default for your new capabilities. Tagging @kvcalvin for clarification, if needed.

@kvcalvin
Copy link

@tanzeli1982 and @thorntonpe: If this feature is intended to be run in v2, then it will need to be incorporated into the CTC model as that is the soil BGC formulation for v2.

@tanzeli1982
Copy link
Contributor Author

@thorntonpe @kvcalvin It was tested on ECA but works for both.

@thorntonpe
Copy link
Contributor

@tanzeli1982 Can you provide more details on what "tested on ECA, but works for both" means? Does the design document need to be modified to reflect this assessment?

@tanzeli1982
Copy link
Contributor Author

@thorntonpe It means that the soil erosion model does not rely on the implementation of any BGC scheme because it uses the general C, N and P state and flux interfaces in data_types/ColumnDataType.F90 and ECA or CTC is not the condition for any part of the code. I will modify the documentation to make it clear.

@thorntonpe
Copy link
Contributor

@tanzeli1982 Thanks for the clarification. Does this PR include soil erosion model parameters that have been set on the basis of comparing simulation output to observations?

@tanzeli1982
Copy link
Contributor Author

@thorntonpe There are three variables (parEro_c1, parEro_c2 and parEro_c3) in the surface data that are soil erosion parameters from calibration. The model data comparison is now in the code review page.

jgfouca pushed a commit that referenced this pull request Jun 25, 2019
Port to new cgd system izumi

Test suite: scripts_regression_tests.py
Test baseline:
Test namelist changes:
Test status:

Fixes:
User interface changes?:

Update gh-pages html (Y/N)?:

Code review:
@bishtgautam
Copy link
Contributor

@tanzeli1982 The merging of the master branch into a feature branch is highly discouraged. Is there a reason you did this merge?

@tanzeli1982
Copy link
Contributor Author

tanzeli1982 commented Jul 3, 2019

@bishtgautam I was not aware of the risk of this operation. How could I revert the commit?

@rljacob
Copy link
Member

rljacob commented Jul 5, 2019

Do a hard reset on your branch to the commit before you did that merge.
git reset --hard 0b9a938

Then force push the branch to github
git push --force origin tanzeli/lnd/elm-erosion-master

@tanzeli1982 tanzeli1982 force-pushed the tanzeli/lnd/elm-erosion-master branch from e81a414 to 0b9a938 Compare July 5, 2019 17:43
@tanzeli1982
Copy link
Contributor Author

@rljacob Thank you very much.

@tanzeli1982
Copy link
Contributor Author

@bishtgautam should we move this PR forward? Kate has approved the design document.

@bishtgautam
Copy link
Contributor

@thorntonpe, @qzhu-lbl, Could you review this PR? thanks

bishtgautam added a commit that referenced this pull request Jul 30, 2019
Add the soil erosion model to ELM. The changes include the implementation
of soil erosion and erosion induced sediment, C, N and P fluxes from land to rivers.

[BFB] and [NML]
@bishtgautam bishtgautam merged commit 0b9a938 into master Jul 31, 2019
bishtgautam added a commit that referenced this pull request Jul 31, 2019
Add the soil erosion model to ELM. The changes include the implementation
of soil erosion and erosion induced sediment, C, N and P fluxes from land to rivers.

[BFB]
[NML]
@rljacob
Copy link
Member

rljacob commented Jul 31, 2019

2 PET tests failed and I don't think this PR can be ruled out as the cause:
PET_Ln9_PS.ne30_oECv3_ICG.A_WCYCL1850S.sandiatoss3_intel.allactive-mach-pet
PET_Ln5.ne4_ne4.FC5AV1C-L.sandiatoss3_intel.allactive-mach-pet

@bishtgautam please run a PET test with an I case that uses this code.

@bishtgautam
Copy link
Contributor

Both PET tests passed on Compy using this feature branch.

@tanzeli1982 tanzeli1982 deleted the tanzeli/lnd/elm-erosion-master branch July 31, 2019 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB Land NML River
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants