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

Make separate instantaneous and non-inst. history tapes #2445

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

slevis-lmwg
Copy link
Contributor

@slevis-lmwg slevis-lmwg commented Mar 28, 2024

Description of changes

I started from issue #1059 and PR #2019 and am restarting here. From closed PR #2019 I preserved a few things:

  • If avgflag /= 'I', time_bounds is present and time = mid of time_bounds
  • Some long_name changes
  • Treating avgflag as a tape (not field) trait for 'I' and 'L' tapes

In this PR we also intend to split all history tapes into instantaneous and non-instantaneous. The CAM group accomplished this in PR ESCOMP/CAM#903 and we plan to maintain consistency in the appearance of CLM and CAM history files.

UPDATE
Due to higher priority, changing "time" to equal the middle of time_bounds will now get done in
#2838
ESCOMP/MOSART#106
ESCOMP/RTM#39

Specific notes

Contributors other than yourself, if any:
@ekluzek @billsacks

CTSM Issues Fixed (include github issue #):
Fixes #1059

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)?
We intend to split all history tapes into instantaneous and non-instantaneous.

...and other mods that I'm preserving from closed PR ESCOMP#2019, such as
- changes to long_names and
- treating avgflag as a tape (not field) trait for 'I' and 'L' tapes
@slevis-lmwg slevis-lmwg self-assigned this Mar 28, 2024
@slevis-lmwg slevis-lmwg added enhancement new capability or improved behavior of existing capability priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations labels Mar 29, 2024
@slevis-lmwg
Copy link
Contributor Author

This far I have tried one test:
ERP_D_P48x1.f10_f10_mg37.IHistClm51Bgc.izumi_nag.clm-decStart
The test completes as expected and shows different answers from dev175 only for the "time" variable.

@wwieder wwieder added this to the CESM3 Answer changing freeze milestone Jun 20, 2024
@wwieder
Copy link
Contributor

wwieder commented Oct 16, 2024

Conceptually, this seems like a good idea. Practically, this seems like a heavy lift.
I wonder how high a priority it should be for the CESM project and the CLM team?
How critical is this kind of consistency for CESM3?
@dlawrenncar and @briandobbins, could you weigh in here?
@billsacks and @phillips-ad, I think you've thought more about this too?

@phillips-ad
Copy link

I think that consistency in the component output of CESM3 is quite important from a user's standpoint. CTSM would be an outlier amongst the 4 major components if these changes were not implemented. Looking at the project page, these changes have already been put into CAM and CICE. MOM came with these settings out of the box. @slevis-lmwg has a couple of pull requests with these changes for MOSART and RTM. I am a bit unclear where CISM is on implementing this. The only component that I know of that will likely not be implementing this is WW.

@billsacks
Copy link
Member

I started to write a reply but @phillips-ad beat me to it. The critical piece of this is fixing time values for time-averaged fields so that the time is in the middle of the averaging period instead of at the end. As @phillips-ad says, it is likely to be particularly confusing if CTSM (and the river components) differs from other components in this respect. The separation of instantaneous from time-averaged fields is not a critical piece of this, but seemed like the best way to support having correct time values for both time-averaged fields and instantaneous fields.

@slevis-lmwg
Copy link
Contributor Author

I started to write a reply but @phillips-ad beat me to it. The critical piece of this is fixing time values for time-averaged fields so that the time is in the middle of the averaging period instead of at the end. As @phillips-ad says, it is likely to be particularly confusing if CTSM (and the river components) differs from other components in this respect. The separation of instantaneous from time-averaged fields is not a critical piece of this, but seemed like the best way to support having correct time values for both time-averaged fields and instantaneous fields.

@billsacks I find the distinction in priority between the "middle of averaging period" task and the "separating instantaneous and non" task very helpful, because the former will take me a few days at most, while the latter will take me a few weeks at least.

@billsacks
Copy link
Member

I find the distinction in priority between the "middle of averaging period" task and the "separating instantaneous and non" task very helpful, because the former will take me a few days at most, while the latter will take me a few weeks at least.

Thanks, that's good to know, @slevis-lmwg . I'm curious what the plan is, then, for instantaneous fields? Will you just let the time be incorrect for those fields for now?

@phillips-ad do you agree that getting time correct for the time-averaged fields is the high priority thing here even if instantaneous fields aren't handled ideally for now?

@phillips-ad
Copy link

Yes I 100% agree, getting the time set to the middle of the averaging period for time-averaged fields is the higher priority.

@samsrabin
Copy link
Collaborator

Just wanted to add: Separating instantaneous and time-averaged fields will probably be an issue for #2061, which adds a namelist option to enable all our history fields and put them all on the h0 file. Depending on which PR comes in first, we'll have to think about how we want to handle that.

@slevis-lmwg
Copy link
Contributor Author

Just wanted to add: Separating instantaneous and time-averaged fields will probably be an issue for #2061, which adds a namelist option to enable all our history fields and put them all on the h0 file. Depending on which PR comes in first, we'll have to think about how we want to handle that.

Thank you for pointing this out @samsrabin

@slevis-lmwg
Copy link
Contributor Author

Based on the recent conversation here, I have opened a new PR #2838 that has the same starting point as this one (#2445). The new PR will not make separate instantaneous and non-instantaneous history tapes.

@slevis-lmwg slevis-lmwg changed the title Make separate instantaneous and non-inst. history tapes; for the latter set time = mid of time_bounds Make separate instantaneous and non-inst. history tapes Oct 18, 2024
@slevis-lmwg slevis-lmwg removed the priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations label Oct 18, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Oct 23, 2024

Thanks, that's good to know, @slevis-lmwg . I'm curious what the plan is, then, for instantaneous fields? Will you just let the time be incorrect for those fields for now?

@billsacks

Yes, we will let the instantaneous fields be incorrect to start with. And then a later more involved change will fix that.

I like the way that @samsrabin framed this when we discussed this. Right now "I" fields are correct, but "A" fields are incorrect. But, we care more about "A" fields (there's more of them output and they are the standard for most things) so getting them corrected at the expense of "I" fields is an improvement.

Reduce outputs from matrixcnOn tests

slevis resolved conflicts:
src/main/histFileMod.F90
@slevis-lmwg

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability
Projects
Status: Todo
6 participants