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 FAN and fertilizer for crops #5981

Conversation

bbye
Copy link
Contributor

@bbye bbye commented Oct 11, 2023

Adds Flow of Agriculture Nitrogen (ammonia emissions) to crop model.
Adds gridded fertilizer for nitrogen and phosphorus.
Adds manure application.

All tests with CN biogeochemistry will have a namelist change.
[NML]
[non-BFB] for crop tests

This adds the first round of the Flow of Agricultural Nitrogen (FAN) code
which includes three new files, two with the FAN algorithms and one with
the data stream.
Adds gridded nitrogen and phosphorus fertilizer. Adds fixed manure
application if FAN is not used. If FAN is on, then uses FAN manure
applications.
This commit allows ammonia emissions from FAN to couple with the atmosphere.
Note that this feature is off by default and has not been tested with the full
atmosphere or chemistry model.
There are several items addressed in this commit to improve the crop model.
1. First it resolves a bug in the crop  model preventing bfb restart
2. Adds the missing components needed to collapse crop types that are not calibrated
into ones that are known to the model. This allows placeholders of cfts so we don't
have to keep changing the surface data files.
3. Resolves an issue with the cropseedc, cropseedn, and cropseedp so they don't keep growing
negative every time the initial seed is given at planting.
This is the last of the FAN subroutine. It updates the mksurfdat routine to
include the gridded fertilizer. Also updates some parameter and surface data
files with the fertilizer. Finally, a test for FAN is included (default mode) only.
Bugfixes to get tests to pass
@bbye bbye added the Land label Oct 11, 2023
@bbye bbye requested a review from evasinha October 11, 2023 01:06
@bbye
Copy link
Contributor Author

bbye commented Oct 11, 2023

The land developer tests all passed, with diffs for all the smallvilleIA tests due to the new fertilizer datasets. All the tests with CN also fail the namelist check because there is a new fan namelist.

A description of the feature can be found at: https://acme-climate.atlassian.net/wiki/spaces/DOC/pages/2237465109/B14+Flow+of+Agriculture+Nitrogen+Design+Document

@bbye bbye changed the title bbye/lnd/integrate fan and fertilizer for crops Add FAN and fertilizer for crops Oct 12, 2023
Copy link
Contributor

@evasinha evasinha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @bbye for getting the FAN implemented in E3SM and submitting the PR.

I only have minor edits for your consideration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please add units for manure_n_transf_col , nh3_otherfert_col, and nh3_total_col
  • Parenthesis are missing or not closed for few units.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this was due to a copy paste, it's fixed now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo - 'ammoniacal'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual spelling (I looked it up to make sure), it refers to the ammonia component of nitrogen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the FanMod.F90 and FanUpdateMod.F90, please consider listing name of all subroutines after contains.

For e.g.,

contains

procedure , public  :: fanInit    

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do this because I think it's generally done when defining a type. All the public subroutines are listed at the beginning of the module and I added some extra description about the flow of the FAN model in each of FanUpdateMod and FanMod.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The listing of public functions is before the "contains" and there is a small block in each of the new files like this one in FanMod.F90.

private
 public update_org_n
 public eval_fluxes_storage
 public update_npool
 public update_4pool
 public update_urea

You might want to add an inline comment for each of those explaining what they do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I went through FanMod and FanUpdateMod and added inline comments to all the public functions to briefly explain what they do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Minor typo - The comment for tan_s3 ! (gN/m2) total ammoniacal N in FAN pool S2 incorrectly calls it FAN pool S2. Same typo was in components/elm/src/biogeochem/CNNitrogenStateType.F90
  2. Initialization of all FAN variables - should it be nan or spval?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the typos are fixed and the initialization was changed to spval.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module has various subroutines and functions. Consider adding short notes on the top that explain the flow through various subroutines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I added more description about the flow of the modules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 9397 to 9430 can all be in a single crop_prog loop instead of five separate if loops for crop_prog.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is now one loop.

Fixes some initialization, loop configuration, typos, and adds some
more text describing FAN module.
Copy link
Contributor

@evasinha evasinha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments have been addressed. I approve the PR for merging to the master.

Add more comments to FanMod and FanUpdateMod
@mingxuanwupnnl
Copy link
Contributor

Nice to see that this feature will be in master. Will this be turned on by default for BGC?

@bbye
Copy link
Contributor Author

bbye commented Nov 17, 2023

@mingxuanwupnnl Sorry, I just saw your comment. This feature won't be on by default in v3 sims because it requires the crop model to be turned on and that won't be on by default in v3. However, we are planning to do some simulations with the crop model on and we can have FAN on with those.

peterdschwartz added a commit that referenced this pull request Nov 21, 2023
…tilizer_for_crops' into next (PR #5981)

Adds Flow of Agriculture Nitrogen (ammonia emissions) to crop model.
Adds gridded fertilizer for nitrogen and phosphorus.
Adds manure application.

All tests with CN biogeochemistry will have a namelist change.
[BFB] except for crop tests.
[NML] diff
@peterdschwartz
Copy link
Contributor

merged to next

@peterdschwartz
Copy link
Contributor

@bbye some tests failed due to missing inputdata file
file not found: stream_fldfilename_fan = /sems-data-store/ACME/inputdata/lnd/clm2/ndepdata/fan_nitrogen_e3sm_f19_c20191010.nc

@peterdschwartz
Copy link
Contributor

@bbye I went ahead and added that file to the data server. Just have to check if any others are missing

@peterdschwartz
Copy link
Contributor

@bbye Also, it seems that .case.setup will still fail before it can even attempt to download the file, so it needs to be copied to every inputdata manually?? If so, it may need to be reworked, and I may just revert it today.

I always get the error message

ERROR: Command: '/global/u2/p/pschwar3/E3SM/components/elm/bld/build-namelist -infile /pscratch/sd/p/pschwar3/fantest/ERS.f19_g16.I1850GSWCNPECACNTBC.pm-cpu_intel.elm-eca_f19_g16_I1850GSWCNPECACNTBC.C.20231122_111411_ugxi8j/Buildconf/elmconf/namelist  -csmdata /global/cfs/cdirs/e3sm/inputdata -inputdata /pscratch/sd/p/pschwar3/fantest/ERS.f19_g16.I1850GSWCNPECACNTBC.pm-cpu_intel.elm-eca_f19_g16_I1850GSWCNPECACNTBC.C.20231122_111411_ugxi8j/Buildconf/elm.input_data_list -ignore_ic_year -namelist " &elm_inparm  start_ymd=00010101  /" -use_case 1850_control  -res 1.9x2.5  -clm_start_type default -envxml_dir /pscratch/sd/p/pschwar3/fantest/ERS.f19_g16.I1850GSWCNPECACNTBC.pm-cpu_intel.elm-eca_f19_g16_I1850GSWCNPECACNTBC.C.20231122_111411_ugxi8j -l_ncpl 48 -r_ncpl 8 -lnd_frac /global/cfs/cdirs/e3sm/inputdata/share/domains/domain.lnd.fv1.9x2.5_gx1v6.090206.nc -glc_nec 0 -co2_ppmv 284.7 -co2_type constant  -ncpl_base_period day  -config /pscratch/sd/p/pschwar3/fantest/ERS.f19_g16.I1850GSWCNPECACNTBC.pm-cpu_intel.elm-eca_f19_g16_I1850GSWCNPECACNTBC.C.20231122_111411_ugxi8j/Buildconf/elmconf/config_cache.xml -bgc bgc -nutrient cnp -nutrient_comp_pathway eca -soil_decomp century -methane  -mask gx1v6' failed with error 'File::Glob::glob() will disappear in perl 5.30. Use File::Glob::bsd_glob() instead. at /global/u2/p/pschwar3/E3SM/components/elm/bld/ELMBuildNamelist.pm line 3674.
ERROR : CLM build-namelist::ELMBuildNamelist::add_default() : file not found: stream_fldfilename_fan = /global/cfs/cdirs/e3sm/inputdata/lnd/clm2/ndepdata/fan_nitrogen_e3sm_f19_c20191010.nc' from dir '/pscratch/sd/p/pschwar3/fantest/ERS.f19_g16.I1850GSWCNPECACNTBC.pm-cpu_intel.elm-eca_f19_g16_I1850GSWCNPECACNTBC.C.20231122_111411_ugxi8j/Buildconf/elmconf'

Attempting to use ./check_input_data --download doesn't help -- maybe due to setup failing before it can generate elm.input_data_list

peterdschwartz added a commit that referenced this pull request Nov 22, 2023
…_and_Fertilizer_for_crops' into next (PR #5981)"

This reverts commit f1eaf60, reversing
changes made to 892e777.
@peterdschwartz
Copy link
Contributor

Reverted this PR for now. Need to investigate how the changes to the namelists are done.

@peterdschwartz
Copy link
Contributor

In E3SM/components/elm/bld/ELMBuildNamelist.pm the add_default function checks if a file exists. Commenting out this check will allow the case to setup and download the missing lnd/clm2/ndepdata/fan_nitrogen_e3sm_f19_c20191010.nc as expected.

Maybe there's a better solution, but having a missing file result in a fatal error seems to contradict the idea behind being able to download files from the data server to begin with.

@rljacob
Copy link
Member

rljacob commented Nov 28, 2023

That sounds like the right solution.

@peterdschwartz
Copy link
Contributor

peterdschwartz commented Nov 30, 2023

Update: There's another error message for a couple of pm-cpu_gnu tests where the fan atm is enabled but use_fan is disabled. I believe I tracked it down to shr_fan_readnl not initializing fan_nh3_to_atm so that if the drv_flds_in file does not exist fan_nh3_to_atm defaults to .true. for this compiler.

Just haven't been able to properly test the fix the past couple of days due to perlmutter being down for maintenance

@rljacob
Copy link
Member

rljacob commented Dec 4, 2023

@peterdschwartz any further progress?

@peterdschwartz
Copy link
Contributor

@rljacob Yes, I did fix the other error message. I'll push the new commit to @bbye branch after I do a test merge and run all the tests on pm gnu

ELMBuilNamelist.pm : removed fatal_error call if missing files so that
they can be downloaded from data server.

shr_fan_mod.F90 : initialized fan_nh3_to_atm to .false. in case
drv_flds_in doesn't exist for certain configs.
@peterdschwartz
Copy link
Contributor

With the last round of testing, there were a few parameter and surfdata_map files that had to be added to the data server, and so far that seems to have fixed all the fails (though still waiting on a few).

Hopefully it's straightforward to push these changes to your fork @bbye

@bbye
Copy link
Contributor Author

bbye commented Dec 5, 2023

Thanks @peterdschwartz! I appreciate your help getting the PR fixed.

peterdschwartz added a commit that referenced this pull request Dec 7, 2023
…rate_FAN_and_Fertilizer_for_crops' into next (PR #5981)""

This reverts commit 8bba792.
peterdschwartz added a commit that referenced this pull request Dec 7, 2023
…tilizer_for_crops' into next (PR #5981)

Adds Flow of Agriculture Nitrogen (ammonia emissions) to crop model.
Adds gridded fertilizer for nitrogen and phosphorus.
Adds manure application.

All tests with CN biogeochemistry will have a namelist change.
[non-BFB] for crop tests.
[NML]
@peterdschwartz
Copy link
Contributor

Merged to next. Sucessfully re-ran the new FAN test to make sure the revert and re-merge worked as intended, so hopefully everything checks out tomorrow

@peterdschwartz peterdschwartz merged commit fa40af6 into E3SM-Project:master Dec 8, 2023
2 checks passed
@peterdschwartz
Copy link
Contributor

Merged to master. Submitted bless requests except for pm-cpu_intel which hasn't finished yet

@bbye
Copy link
Contributor Author

bbye commented Dec 8, 2023

Thank you Peter for your help getting this PR in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants