C-based harvest in ELM-FATES #5106

sshu88 · 2022-08-02T21:52:20Z

This pull request contains modification in E3SM corresponding to another FATES PR #888.
In detail, necessary input to enable FATES C-based harvest from ELM is passed to FATES through bc_in. Output from FATES used to calculate land use emission and net biosphere productivity is passed to ELM through bc_out.

rljacob · 2022-08-03T00:07:10Z

The PR description should describe the code changes and not just point to another PR description.

rljacob · 2022-08-25T17:28:57Z

Needs action and review from @sshu88 @rgknox @bishtgautam

rljacob · 2022-09-15T17:24:23Z

Still needs action and review from @sshu88 @rgknox @bishtgautam

rljacob · 2022-10-06T17:34:57Z

telecon notes: Waiting for a FATES update before removing WIP.

glemieux · 2022-10-10T16:47:09Z

components/elm/src/biogeophys/BalanceCheckMod.F90

+             !if ( (errsol(p) /= spval) .and. (abs(errsol(p)) > 1.e-7_r8) ) then
+             ! solar radiation balance error is high when running FATES
+             ! adjust the threshold to 5.e-7
+             if ( (errsol(p) /= spval) .and. (abs(errsol(p)) > 5.e-7_r8) ) then


@sshu88 is this change to try and avoid the water balance error? @rgknox and I recommend that we not change this in e3sm master branch. It should be reverted to the old value.

rgknox · 2022-10-10T16:50:09Z

components/elm/src/biogeophys/BalanceCheckMod.F90

+             !if ( (errsol(p) /= spval) .and. (abs(errsol(p)) > 1.e-7_r8) ) then
+             ! solar radiation balance error is high when running FATES
+             ! adjust the threshold to 5.e-7
+             if ( (errsol(p) /= spval) .and. (abs(errsol(p)) > 5.e-7_r8) ) then


If we bump this value up to suppress warnings, that seems fine for a FATES run. We are very aware that an audit needs to be done to reduce these fine errors in radiation scattering. However, we may want to only apply it to a fates governed patch. This "is_fates" can tell us if we are on a fates patch and if we should apply different logic:
https://github.com/E3SM-Project/E3SM/blob/v2.0.0/components/elm/src/data_types/VegetationType.F90#L70

I have revised the code as suggested. Thank you @rgknox @glemieux .

rljacob · 2022-11-03T17:28:32Z

telecon notes: still waiting for FATES PR.

rljacob · 2022-11-09T23:22:30Z

@rgknox this is waiting on a FATES PR and then I guess an update of FATES to E3SM.

sshu88 · 2023-01-04T22:21:16Z

@bishtgautam We can go ahead and merge the work once the FATES part is merged.

glemieux · 2023-01-04T22:31:56Z

@bishtgautam We can go ahead and merge the work once the FATES part is merged.

@bishtgautam I'm working on the final deconflict and test on the fates-side right now.

fates-nutrient coupling api changes

rgknox · 2023-01-06T23:17:58Z

The changes just merged into this branch, in 13d0f5a , were originally overlooked, but nonetheless required for compatibility with fates API 25.

glemieux

One question about the hardcoded allow_harvest_bypass_criteria

glemieux · 2023-01-18T00:02:41Z

components/elm/src/main/elmfates_interfaceMod.F90

        end if

        call set_fates_ctrlparms('use_lu_harvest',ival=pass_lu_harvest)
+        call set_fates_ctrlparms('use_harvest_bypass_criteria',ival=pass_harvest_bypass_criteria)


I don't see any setup for this on the fates side pr 888. Is this a future feature? I'm assuming so given that it's hard coded to false just above this.

It's an old flag. I forgot to delete this line in ELM side.

Ok, cool. In that case would you push an update to remove it please?

glemieux · 2023-01-18T18:27:17Z

components/elm/src/main/elmfates_interfaceMod.F90

-   use dynHarvestMod          , only : num_harvest_vars, harvest_varnames
+   use dynHarvestMod          , only : num_harvest_vars, harvest_varnames, wood_harvest_units
   use dynHarvestMod          , only : harvest_rates ! these are dynamic in space and time
   use dynHarvestMod          , only : num_harvest_vars, harvest_varnames, wood_harvest_units


@sshu88 it looks like there is some duplicate code here as well.

Thanks for checking.

glemieux

Just one more minor piece of code cleanup.

glemieux · 2023-01-18T18:49:11Z

@sshu88 would you remove the "[WIP]" from the title now?

glemieux · 2023-01-19T22:18:26Z

@bishtgautam we still need to update the pointer to the fates side, but the associated PR hasn't been merged yet. I'll give you a heads up when that happens

bishtgautam · 2023-01-19T23:56:05Z

Are the existing tests cover the new physics? Or, should we add a new test?

glemieux · 2023-01-24T17:53:01Z

Are the existing tests cover the new physics? Or, should we add a new test?

We should add new tests, but I would like to defer that to a future PR in which we review and update all of the fates tests for the land developer suite.

glemieux

I noticed one change that affects non-fates runs that needs to be addressed.

glemieux · 2023-01-27T00:02:02Z

components/elm/src/data_types/ColumnDataType.F90

+       this%prod10c(begc:endc) = spval
+       call hist_addfld1d (fname='PROD10C', units='gC/m^2', &
+            avgflag='A', long_name='10-yr wood product C', &
+            ptr_col=this%prod10c, default='inactive')
+
+       this%prod100c(begc:endc) = spval
+       call hist_addfld1d (fname='PROD100C', units='gC/m^2', &
+            avgflag='A', long_name='100-yr wood product C', &
+            ptr_col=this%prod100c, default='inactive')
+


@sshu88 was this a temporary change to make sure that you could read the prod10c and prod100c for when fates was running?

These are two outputs I added to the history output file for checking the change of product pool size. So it is a temporary change. But the product pool size shall be a basic output in land surface model when considering anthropogenic disturbances.

To clarify, does this need to be available when fates is running? The current issue is that the same call is made just below behind a .not. use_fates check, which means this will get called twice and cause a build failure for non-fates runs. If it's not necessary for fates right now, would you mind removing these lines? Otherwise, the part that is behind the .not. use_fates check could be removed.

I have removed the part below .not. use_fates check.

glemieux · 2023-01-28T00:32:28Z

I'm running the land-developer tests and seeing two unexpected failures. I haven't looked into them much yet, but one is a fates smoke test failing during runtime in PhosphorusDynamicsMod and the other is a model build failure for ERS.ne30pg2_r05_EC30to60E2r2.GPMPAS-JRA.cori-haswell_intel.mosart-rof_ocn_2way.

folder location (Cori):

/global/homes/g/glemieux/cori-scratch/e3sm-tests/pr5106-merge.fates.cori-haswell.intel.Ce0716a090f-F25047167

Note I'm using Cori as perlmutter is down right now.

…ad been used only for nutrient plant uptake

rgknox · 2023-01-31T02:51:13Z

I submitted some updates to @sshu88 's branch to resolve the issues with the fates_eca test. Tests were passing on Cori. Should update once we get it approved and checked.

fixes for nutrient enabled fates

glemieux · 2023-01-31T22:35:57Z

@sshu88 I created a PR to your branch to update the fates pointer now that NGEET/fates#888 is integrated on the fates-side.

@bishtgautam I ran land developer tests on Cori and all the tests except ERS.ne30pg2_r05_EC30to60E2r2.GPMPAS-JRA.gcp12_gnu.mosart-rof_ocn_2way passed (due to #5412).

Update fates submodule pointer to carbon harvest compatible tag

glemieux · 2023-02-02T20:12:34Z

@bishtgautam this can be closed per #5429.

sshu3 added 6 commits February 22, 2022 07:41

Add LUC fluxes in ELM-FATES interface.

975c41a

Removed pprodharv10 from bc_in.

e60d108

Merge branch 'master' into fatesluc

be484a5

Change the maximum numbers of patches to 20.

17bb955

Revise back to 14 patches version from 'flex_numpatch'.

f50460d

Revised ELM C fluxes diagnostic outputs.

b00fb40

glemieux mentioned this pull request Aug 2, 2022

[WIP] Pass harvest Carbon from FATES to ELM #5043

Closed

sshu88 changed the title ~~C-based harvest in ELM-FATES~~ [WIP] C-based harvest in ELM-FATES Aug 2, 2022

rljacob added the Land label Aug 3, 2022

rljacob added the FATES label Aug 3, 2022

rljacob assigned bishtgautam Aug 3, 2022

rljacob requested a review from rgknox August 3, 2022 00:20

sshu3 added 4 commits August 29, 2022 13:05

Removed unnecessary changes.

3da7482

Revert changes in config_pesall.xml and .gitignore

f69a848

Revert changes in testmod files.

12be3a6

Added a missing blank line in testmod file.

b322f08

glemieux requested changes Oct 10, 2022

View reviewed changes

rgknox reviewed Oct 10, 2022

View reviewed changes

sshu3 added 2 commits October 10, 2022 12:08

Revised the solar radiation imbalance threshold for FATES.

362fecf

Remove the confusing IF-ELSE logic.

9b49c52

glemieux mentioned this pull request Oct 11, 2022

Deconflict and cleanup Harvest Carbon PR glemieux/E3SM#4

Closed

3 tasks

glemieux mentioned this pull request Dec 3, 2022

[Land Use] C-based wood harvest NGEET/fates#888

Merged

4 tasks

rgknox added 2 commits December 12, 2022 13:45

Update interface code to allow FATES API 25

f165e40

Update FATES submodule pointer to sci.1.61.0_api.25.0.0

60ee280

rgknox and others added 3 commits January 6, 2023 16:19

fates-nutrient coupling api changes

13d0f5a

small update to a comment

ed780f8

Merge pull request #4 from rgknox/fatesluc_cbasedhrv_v2_allocationfixes

d215068

fates-nutrient coupling api changes

glemieux reviewed Jan 18, 2023

View reviewed changes

Update elmfates_interfaceMod.F90

7480098

glemieux reviewed Jan 18, 2023

View reviewed changes

glemieux approved these changes Jan 18, 2023

View reviewed changes

Update elmfates_interfaceMod.F90

3730a54

sshu88 changed the title ~~[WIP] C-based harvest in ELM-FATES~~ C-based harvest in ELM-FATES Jan 18, 2023

glemieux reviewed Jan 27, 2023

View reviewed changes

Update ColumnDataType.F90

cbc03fb

rgknox added 3 commits January 30, 2023 17:52

Removed a deprecated mapping between fates and elm soil layers that h…

45d053d

…ad been used only for nutrient plant uptake

Updated pftvarcon to read in phosphorus parameters when fates is on

1beed82

Added a div0 protection to phosphorus cycling

2e05202

sshu88 and others added 2 commits January 30, 2023 19:32

Merge pull request #5 from rgknox/fatesluc_cbasedhrv_v2

1b3257f

fixes for nutrient enabled fates

update fates submodule pointer to carbon harvest compatible tag

4d13737

Merge pull request #6 from glemieux/fatesluc_cbasedhrv_v2-fatestag

076299a

Update fates submodule pointer to carbon harvest compatible tag

glemieux mentioned this pull request Feb 2, 2023

Enable C-based harvest in ELM-FATES and update to FATES API 25 #5429

Merged

bishtgautam closed this Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C-based harvest in ELM-FATES #5106

C-based harvest in ELM-FATES #5106

sshu88 commented Aug 2, 2022 •

edited

Loading

rljacob commented Aug 3, 2022

rljacob commented Aug 25, 2022

rljacob commented Sep 15, 2022

rljacob commented Oct 6, 2022

glemieux Oct 10, 2022

rgknox Oct 10, 2022

sshu88 Oct 10, 2022

rljacob commented Nov 3, 2022

rljacob commented Nov 9, 2022 •

edited

Loading

sshu88 commented Jan 4, 2023

glemieux commented Jan 4, 2023

rgknox commented Jan 6, 2023

glemieux left a comment

glemieux Jan 18, 2023

sshu88 Jan 18, 2023

glemieux Jan 18, 2023

glemieux Jan 18, 2023

sshu88 Jan 18, 2023

glemieux left a comment

glemieux commented Jan 18, 2023

glemieux commented Jan 19, 2023

bishtgautam commented Jan 19, 2023

glemieux commented Jan 24, 2023 •

edited

Loading

glemieux left a comment

glemieux Jan 27, 2023

sshu88 Jan 27, 2023

glemieux Jan 27, 2023

sshu88 Jan 27, 2023

glemieux commented Jan 28, 2023

rgknox commented Jan 31, 2023 •

edited

Loading

glemieux commented Jan 31, 2023

glemieux commented Feb 2, 2023 •

edited

Loading

C-based harvest in ELM-FATES #5106

C-based harvest in ELM-FATES #5106

Conversation

sshu88 commented Aug 2, 2022 • edited Loading

rljacob commented Aug 3, 2022

rljacob commented Aug 25, 2022

rljacob commented Sep 15, 2022

rljacob commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rljacob commented Nov 3, 2022

rljacob commented Nov 9, 2022 • edited Loading

sshu88 commented Jan 4, 2023

glemieux commented Jan 4, 2023

rgknox commented Jan 6, 2023

glemieux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemieux left a comment

Choose a reason for hiding this comment

glemieux commented Jan 18, 2023

glemieux commented Jan 19, 2023

bishtgautam commented Jan 19, 2023

glemieux commented Jan 24, 2023 • edited Loading

glemieux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemieux commented Jan 28, 2023

rgknox commented Jan 31, 2023 • edited Loading

glemieux commented Jan 31, 2023

glemieux commented Feb 2, 2023 • edited Loading

sshu88 commented Aug 2, 2022 •

edited

Loading

rljacob commented Nov 9, 2022 •

edited

Loading

glemieux commented Jan 24, 2023 •

edited

Loading

rgknox commented Jan 31, 2023 •

edited

Loading

glemieux commented Feb 2, 2023 •

edited

Loading