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 FATES land use change module to pass LUH2 data to FATES #5760

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

glemieux
Copy link
Contributor

@glemieux glemieux commented Jun 13, 2023

This pull request adds a new module, the structure of which is adopted from the dyn_subgrid code,
to enable e3sm to ingest minimally processed luh2 data to be passed to FATES.
A new namelist variable is introduced use_fates_luh, to engage the module functionality,
which is off by default. The luh2 dataset to be ingested is a minimally processed concatenation
of the luh2 state, transition and management datasets.

These changes also include a minor bug fix discovered in the process of developing the code,
in which if the number of FATES patches being defined by the user in the fates parameter file is greater than the max_patch_per_col elm variable, would result in a BalanceCheck error.

To be coordinated with NGEET/fates#1040

[nonBFB] for FATES

@bishtgautam
Copy link
Contributor

@glemieux Can we add a new test for this PR?

@glemieux glemieux changed the title Add FATES land use change module to pass LUH2 data to FATES WIP: Add FATES land use change module to pass LUH2 data to FATES Jun 27, 2023
@glemieux glemieux changed the title WIP: Add FATES land use change module to pass LUH2 data to FATES [WIP] Add FATES land use change module to pass LUH2 data to FATES Jun 27, 2023
@glemieux
Copy link
Contributor Author

Status update: there is a forthcoming update to the fates luh2 data pipeline NGEET/fates#1032 that will result in an update to the default fates landuse data set. As such I've added "WIP" to this PR.

@rljacob
Copy link
Member

rljacob commented Jul 6, 2023

status: probably another month waiting on FATES refactor.

@rljacob
Copy link
Member

rljacob commented Aug 10, 2023

Need #5849 and also still WIP.

@rljacob
Copy link
Member

rljacob commented Sep 7, 2023

#5849 was merged. Status now?

@rljacob
Copy link
Member

rljacob commented Sep 21, 2023

@glemieux is this ready?

@glemieux
Copy link
Contributor Author

glemieux commented Sep 21, 2023

@glemieux is this ready?

@rljacob This is still WIP and waiting on NGEET/fates#1040 updates. I'm actively working on this.

@rljacob
Copy link
Member

rljacob commented Oct 12, 2023

update: still waiting on fates PR. Another couple of weeks.

@rljacob
Copy link
Member

rljacob commented Nov 16, 2023

update: still WIP. 2 others will need to be merged before this.

@rljacob
Copy link
Member

rljacob commented Dec 4, 2023

@glemieux can you list in a comment the PRs this is waiting on?

@glemieux
Copy link
Contributor Author

glemieux commented Dec 4, 2023

@rljacob it needs to be staged after #6018 and #6027, both which need review and are undergoing some testing. Ultimately, it's also waiting on NGEET/fates#1040, but this should be integrated within the next week.

This PR needs to have some updates brought in, but is close to not being "WIP".

Copy link
Contributor

@rgknox rgknox left a comment

Choose a reason for hiding this comment

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

@glemieux and I are discussing some cosmetic things and checking on a setting in the namelist build perl code, but otherwise I think this PR looks good. @glemieux plans to add a test for this as well

@rgknox
Copy link
Contributor

rgknox commented Feb 5, 2024

FYI: The next FATES API (33) is fairly simple, but I will hold on making that until this branch is re-based, since I have to build it on top of this one.

@glemieux
Copy link
Contributor Author

glemieux commented Feb 5, 2024

  • update fates tag
  • add landuse testmod to fates test suite
  • remove use_cn return check from landusemod
  • add fluh_timeseries to setup_cmdl_fates_mode

@glemieux glemieux force-pushed the lnd/fates-luh2 branch 2 times, most recently from 5d54629 to 0fe12be Compare February 7, 2024 21:04
Enable the host land model to input and interpolate raw
land use harmonization data (LUH2) states and transitions
to be passed to FATES.
@glemieux glemieux changed the title [WIP] Add FATES land use change module to pass LUH2 data to FATES Add FATES land use change module to pass LUH2 data to FATES Feb 7, 2024
@glemieux
Copy link
Contributor Author

glemieux commented Feb 7, 2024

@peterdschwartz @rgknox I've taken care of the suggested changes and rebased this onto master now that #6027 is in. I'm going to start regression testing on this today and I've removed the '[WIP]'.

@glemieux
Copy link
Contributor Author

@peterdschwartz regression testing on perlmutter with the e3sm_land_developer suite is complete and all expected non-fates tests are B4B. The qian smoke test from #6192 is failing, but that's because I hadn't rebased since last week. I've got a local rebased branch with #6219 included to make sure its not failing for a different reason. All fates test pass with expected DIFFs in non-landuse run modes since the tag update also includes a couple fates scientific bug fixes since sci.1.68.2_api.31.0.0.

@peterdschwartz
Copy link
Contributor

peterdschwartz commented Feb 16, 2024

@rgknox @bishtgautam This is ready to review then.

Edit: Needs to be re-approved after rebase.

@glemieux
Copy link
Contributor Author

Just a quick update to note that rebasing against the latest master resulted in SMS.r05_r05.I1850ELMCN.pm-cpu_....elm-qian_1948 passing.

@peterdschwartz
Copy link
Contributor

Tested on chrysalis and everything came back as expected. This can go in once it's got one approval

Copy link
Contributor

@bishtgautam bishtgautam left a comment

Choose a reason for hiding this comment

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

I have a few minor comments. However, a major comment is: Shouldn't there be a new compset (say I20TRELMFATES) in which the transient LU data is used by FATES?

@@ -565,6 +565,11 @@ lnd/clm2/surfdata_map/surfdata_conusx4v1_simyr2000_c160503.nc</fsurdat>
<flanduse_timeseries hgrid="ne30np4" rcp="2.6" sim_year_range="1850-2100"
use_crop=".false." >lnd/clm2/surfdata_map/surfdata.pftdyn_ne30np4_rcp2.6_simyr1850-2100_c130524.nc</flanduse_timeseries>

<!-- Land Use Harmonization unified data sets for dynamic FATES land use change -->
<fluh_timeseries hgrid="4x5" sim_year_range="1850-2015" use_fates=".true."
>lnd/clm2/surfdata_map/fates-sci.1.68.3_api.31.0.0_tools.1.0.1/LUH2_states_transitions_management.timeseries_4x5_hist_simyr1850-2015_c231101.nc</fluh_timeseries>
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. This file is not on the E3SM input data server.
  2. FATES's SCI/API version number will continue to get updated. This would mean we would have many fates-sci* directories. How we have a single dir (e.g. lnd/clm2/surfdata_map/fates) and the metadata of the LUH2 file can include the information regarding the SCI/API version. In the F90 code, you can also include a sanity check regarding the SCI/API version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. @bishtgautam this file is fairly large. I submitted a request about 2 weeks ago to get access to the ANL servers using the procedure laid out on confluence so that I could load this and future updates directly. I haven't seen movement on that request. In the meantime, the data is on perlmutter; is this something you could upload for me via globus?
  2. That should work. The file structure here was the recommended by ctsm folks, although I can't remember the rationale currently. I'll definitely add the fates sci/api version to the data file meta data. That said, the meta data check isn't really needed for usage here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -127,7 +127,7 @@ attributes from the config_cache.xml file (with keys converted to upper-case).
<!-- ================================================================== -->
<!-- FATES default parameter file -->
<!-- ================================================================== -->
<fates_paramfile >lnd/clm2/paramdata/fates_params_api25.5.0_12pft_c230710.nc</fates_paramfile>
<fates_paramfile >lnd/clm2/paramdata/fates_params_api.32.0.0_12pft_c231215.nc</fates_paramfile>
Copy link
Contributor

Choose a reason for hiding this comment

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

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 @bishtgautam I'll email you the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, FATES also contributes code to CTSM, so it is on NCAR's server. In that case, we don't need to put on the E3SM server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice, I didn't realize that! Thanks @peterdschwartz

@rgknox rgknox self-requested a review February 21, 2024 01:26
@glemieux
Copy link
Contributor Author

glemieux commented Feb 22, 2024

However, a major comment is: Shouldn't there be a new compset (say I20TRELMFATES) in which the transient LU data is used by FATES?

@bishtgautam Eventually we should have a compset for this, but with this PR I don't think its ideal as we're going to be bringing in a fates landuse v2 change in the next month or so. I think with the v2 update we will want to facilitate easier use of the option through the compset.

On a related note and by comparison, I think its definitely worth creating a fates SP mode compset with a future PR since that run mode is more mature.

@bishtgautam bishtgautam self-requested a review February 22, 2024 21:28
@peterdschwartz
Copy link
Contributor

Still waiting for the repo to be open.

peterdschwartz added a commit that referenced this pull request Mar 5, 2024
…5760)

This pull request adds a new module, the structure of which is adopted from the dyn_subgrid code,
to enable e3sm to ingest minimally processed luh2 data to be passed to FATES.
A new namelist variable is introduced use_fates_luh, to engage the module functionality, which is off by default.
The luh2 dataset to be ingested is a minimally processed concatenation of the luh2 state, transition and management datasets.

These changes also include a minor bug fix discovered in the process of developing the code,
in which if the number of FATES patches being defined by the user in the fates parameter file
is greater than the max_patch_per_col elm variable, would result in a BalanceCheck error.

To be coordinated with NGEET/fates#1040

[nonBFB] for FATES
@peterdschwartz
Copy link
Contributor

merged to next

@peterdschwartz peterdschwartz merged commit 069c226 into E3SM-Project:master Mar 6, 2024
10 checks passed
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.

5 participants