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

Include PRISM precipitation datm streams #219

Merged
merged 20 commits into from
May 3, 2023
Merged

Conversation

TeaganKing
Copy link
Collaborator

@TeaganKing TeaganKing commented Feb 21, 2023

Description of changes

This PR includes changes that allow PRISM precipitation to be used as a new datm stream.

Updates to cime_config/stream_cdeps.py allow stream names to be variables. Changes in datm/cime_config/namelist_definition_datm.xml include a new stream for PRECIP so that other variables can use the regular NEON datm stream while PRECIP uses this PRISM datm stream; note that this is a user interface change. The file datm/cime_config/stream_definition_datm.xml now includes an additional datm mode for PRISM.

Using PRISM precipitation instead of NEON precipitation does have a substantial impact on CTSM output (eg. latent heat flux biases).

Note:

This PR's functionality also depends on the PRISM PRECIP CTSM PR, PRISM PRECIP ESMCI PR, and input data availability.

Contributors other than yourself, if any: @jedwards4b @wwieder @ekluzek

CDEPS Issues Fixed: #212

To Do:

  • Implement namelist switches to allow users to easily choose between using PRISM or NEON precipitation
  • Determine where to store initial condition files. Also, I have some scripts for processing the data directly from PRISM; we should determine if we want to include these auxiliary scripts somewhere.
  • Think about generalizing to reading in other 'alternative' input data (eg, simulations from ESPWG)

@ekluzek ekluzek added enhancement New feature or request CESM Only labels Feb 21, 2023
@ekluzek
Copy link
Collaborator

ekluzek commented Feb 21, 2023

@jedwards4b could you increase @TeaganKing permission level so that she can assign herself and work with lables on PR's and issues? I think this is the "triage" level.

Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

@TeaganKing so good to see this work started! It's important to have this constructed in a way that the user won't have to comment out a part of their user files. This should be something that can be done. We may need to have a few of us look at it together to figure out how to do it.

datm/cime_config/stream_definition_datm.xml Outdated Show resolved Hide resolved
@ekluzek
Copy link
Collaborator

ekluzek commented Feb 21, 2023

@TeaganKing is this meant to be work in progress that you are going to gradually add too? Or is the intention to do this as a first of several steps?

@TeaganKing
Copy link
Collaborator Author

@ekluzek this is meant to be a WIP PR. I'll update the title.

@TeaganKing TeaganKing changed the title Include PRISM precipitation datm streams [WIP] Include PRISM precipitation datm streams Feb 21, 2023
@wwieder
Copy link

wwieder commented Feb 27, 2023

From talking to @ekluzek it sounds like we'll first update the CTSM externals, then this PR can build off of those updates?

@ekluzek
Copy link
Collaborator

ekluzek commented Feb 27, 2023

Yes, @billsacks PR to update externals should be done first. Then CDEPS will be closer to the tag with the changes needed to pull this in.

@ekluzek
Copy link
Collaborator

ekluzek commented Mar 6, 2023

As already mentioned by @TeaganKing this PR is dependent on CTSM changes. The CTSM tag that brings this in will bring them in together. We will also coordinate CESM tags to note the dependence so that they come in together for the CESM alpha tag that includes both. Since NEON and NEON PRISM precip is a specialized use, it's probably OK for the CDEPS tag to come in independently, but we will try to make sure the CESM and CTSM are appropriately coordinated.

@TeaganKing
Copy link
Collaborator Author

TeaganKing commented Mar 7, 2023

Thanks for mentioning that, @ekluzek .

After meeting with @wwieder and @ekluzek , we came up with a few more implementation details. The main 'switch' should be implemented via allowing the existing xml variable CLM_USRDAT_NAME to be either NEON or NEON.PRISM. This variable is set in CTSM/cime_config/usermods_dirs/NEON/defaults/shell_commands. This will indicate whether we are using the PRISM precip data (when set to NEON.PRISM) or the NEON precip data (default, as in NEON). @jedwards4b had previously suggested adding code in cdeps/datm/cime_config/buildnml to update stream_definition_datm.xml, which could perhaps use the CLM_USRDAT_NAME as a switch.

A few other notes that we discussed:

  • We'd like to add PRISM processing (csv to nc file, metadata additions, download instructions) scripts to ctsm tools/contrib (and update the README accordingly)
  • NCAR should host PRISM data
  • It would be helpful for NEON to host initial condition files, but we will need some way of defining where to get this on the NEON side (eg in run_neon)

This is also relevant for the CTSM PR.

Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

Sounds good. Thank you

@ekluzek
Copy link
Collaborator

ekluzek commented Mar 28, 2023

In working with @TeaganKing we realize we should add both a test for NEON and one for NEON.prism to the cdeps test list. I'll propose what that should look like.

@ekluzek
Copy link
Collaborator

ekluzek commented Mar 28, 2023

In our discussion we also thought the best way forward would be to have the NEON streams variable list always NOT include precip. And then the NEON stream has a third stream for NEON precip, and then the PRISM precip stream. So you have to pick either NEON precip or PRISM precip.

@wwieder
Copy link

wwieder commented Mar 28, 2023

I'm fine with this last option "So you have to pick either NEON precip or PRISM precip", but wonder if we can default to using the NEON precip if users don't make a choice? It seems like this would make the system easier to use for novice users.

@ekluzek
Copy link
Collaborator

ekluzek commented Mar 28, 2023

Yes the user will set this up NEON will get the NEON precip. And I'd they choose NEON prism will get prism precip. The user chooses this in clm usrdat name. So it's a single choice for them. The details here will be hidden for most users.

@TeaganKing
Copy link
Collaborator Author

Right, I think we can still have CLM_USRDAT_NAME be either 'NEON' or 'NEON.PRISM'. 'NEON' will result in using two datm streams: NEON and NEON.NEON_PRECIP. 'NEON.PRISM' will result in using NEON and NEON.PRISM_PRECIP. In both cases, all non-precipitation vars are NEON-based and will be in the NEON datm stream.

@wwieder
Copy link

wwieder commented Mar 28, 2023

Perfect, this seems appropriate.

@TeaganKing
Copy link
Collaborator Author

Following up on the conversation with @ekluzek from yesterday, the third 'NEON_PRECIP' stream has been added. Cases are now running successfully with NEON precipitation and other NEON vars when CLM_USRDAT_NAME is set to 'NEON'. However, I'm still trying to determine why the PRISM_PRECIP stream is now being built with incorrect file names (but correct var names and stream names, eg /glade/work/tking/CTSM/tools/site_and_regional/TEAK.transient/CaseDocs/datm.streams.xml). So, the next steps will be to debug the NEON.PRISM_PRECIP.$NEONSITE datafiles, include tests for CTSM and CDEPS, and provide updated inputdata using /glade/p/cesmdata/cseg/inputdata/rimport.

@wwieder
Copy link

wwieder commented Apr 7, 2023

I think we also have to push files to NEON, but this can wait until we have everything ready on the CLM side.

@TeaganKing
Copy link
Collaborator Author

Hi @jedwards4b -- I was discussing with @ekluzek and we were thinking it would be most reasonable to host the PRISM precipitation data on the NCAR side rather than the NEON side. Does it seem reasonable to make a new 'v2' directory under /glade/p/cesmdata/cseg/inputdata/atm/cdeps which could hold this data? I think there's a 'v2' directory on the NEON side, but not on the NCAR side. Would these directories be merged when pulling down the data for running a case?

@wwieder in response to your comment above, were you thinking we'd push the PRISM data or other updated files to NEON?

@wwieder
Copy link

wwieder commented Apr 10, 2023

We need some way to store and access initial condition files and history output for PRISM forced simulations on NEON's AWS. Once Dawn, Dave, Mike et all decide how to best organize this on the NEON side, maybe @jedwards4b can maybe help clarify how we should be pulling these restart files. I'll start an email thread on this.

@wwieder
Copy link

wwieder commented Apr 10, 2023

I also wonder if the v2.PRISM data we're hosting on the NCAR side should specify PRISM, to avoid confusion with the NEON v2 data we're also using for other DATM input streams?

@ekluzek
Copy link
Collaborator

ekluzek commented Apr 10, 2023

That's a good point @wwieder you should have a subdirectory to distinguish this from the NEON data named PRISM or some such to show that it's being managed by us in CESM rather than by NEON.

@jedwards4b
Copy link
Contributor

/glade/p/cesmdata/cseg/inputdata/atm is intended to be a mirror of our svn inputdata server - would you want to import these files to svn? If not would a cgd based location be better than a cisl one?

@TeaganKing
Copy link
Collaborator Author

I was thinking it would be useful to have a consistent location for the PRISM and NEON data on the svn server, and we wanted to have a more permanent location for the inputdata than my working directory. We actually have already imported the data to the svn inputdata server. That said, if there is a reason that another location is preferred, I would certainly be open to your thoughts!

@wwieder
Copy link

wwieder commented Apr 17, 2023

It seems like @jedwards4b's suggestion of using /glade/p/cesmdata/cseg/inputdata/atm would be appropriate for these data? What you you think, @ekluzek?

@TeaganKing
Copy link
Collaborator Author

The data is currently here: /glade/p/cesmdata/cseg/inputdata/atm/cdeps/PRISM and on SVN (using the rimport script to put data from inputdata on svn)

@jedwards4b
Copy link
Contributor

So my concern here is that the rimport process requires a manual intervention each time data is added - do we think that this will be low frequency? Or will we be swamped by the number of times we will need to do this? I'm fine with it if you don't think we need to do it often, but if it seems like it will need frequent updates than maybe we should look for another way.

@TeaganKing
Copy link
Collaborator Author

Thanks for clarifying your concern; that's helpful to understand where you're coming from. As far as I'm aware, it doesn't seem like this will need frequent updating (let me know if other folks think otherwise). I also now have permission to run the rimport scripts and update svn, so we won't need to bother anyone in order to make updates if they are needed.

@ekluzek
Copy link
Collaborator

ekluzek commented Apr 17, 2023

@TeaganKing and @jedwards4b I imagine you will need to update this data yearly as you'll want to add the latest years worth of data to the end of time-series. That's just adding a new file at the end of the listing with the new year. So that seems reasonable to me.

A problem would be if you needed to update the current PRISM data for any of the current years. But, I imagine that won't need to be done?

@wwieder
Copy link

wwieder commented Apr 17, 2023

I would assume that we'd just append new years of data onto these data and not have to change the existing data.

@@ -236,7 +236,6 @@
<file first_year="$DATM_YR_START" last_year="$DATM_YR_END">$DIN_LOC_ROOT/atm/cdeps/v1/$NEONSITE/%ym.nc</file>
Copy link
Collaborator Author

@TeaganKing TeaganKing Apr 18, 2023

Choose a reason for hiding this comment

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

We only have a 'v1' on /glade/p/cesmdata/cseg/inputdata/atm/cdeps. However, should both of the NEON streams (NEON and NEON.PRECIP) be pulling in 'v2' or 'v3' data from the NEON side? @wwieder , this was the potential issue I was referring to in our conversation on Monday.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jedwards4b after discussing with @wwieder , it sounds like this gets overwritten to the latest version elsewhere, but should we update to 'v2'?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if it's only being overwritten to change v1 to v2 then I would both update this to v2 AND get rid of the code that is overwriting this setting. Make sense?

Copy link

Choose a reason for hiding this comment

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

Just to clarify, we want the feature to continue drawing from the latest version if no version is specified (e.g. some NEON sites have v3 data now!).

I get confused because the run_neon script has a feature that allows users to specify what version is being used, but the cdeps code seems hard coded to v1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. I agree that logically makes sense, but I'm not really sure where this actually gets updated, and I think we want to take the latest version (v3 is coming online soon/partially available for one site). For the sake of getting this PR merged and maintaining flexibility, I think it may make the most sense to add a comment to clarify this in CDEPS, and potentially tweak run_neon in the future.

@TeaganKing TeaganKing requested a review from ekluzek April 18, 2023 18:59
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

This is ready to come in now.

@ekluzek
Copy link
Collaborator

ekluzek commented May 3, 2023

@TeaganKing can you change the title so that it doesn't have the WIP in the name now that this is ready to go?

@jedwards4b could you merge this in? Or would it be OK for me to do that, since this only affects NEON cases? I'm happy to do more testing, it just doesn't seem like there's anything needed since it's so restricted to NEON.

@jedwards4b jedwards4b changed the title [WIP] Include PRISM precipitation datm streams Include PRISM precipitation datm streams May 3, 2023
@jedwards4b jedwards4b merged commit f2c4fe1 into ESCOMP:main May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CESM Only enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add capability to switch NEON sites to use PRISM forcing data
4 participants