-
Notifications
You must be signed in to change notification settings - Fork 306
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
Do not allow instantaneous fields on same history tape as other fields #2019
Conversation
@slevis-lmwg SNOW_PERSISTANCE is the length in days of how long snow has persisted on that column. So there is a good reason for it to be the instantaneous value at the output frequency. I do see why you are simplifying it though as well. But, the average value really doesn't sound like a useful value for this field. This field has been an instantaneous value since at least water types were broken out 5 years ago. I suppose you could sort of cheat and use the max value for the averaging period and that would be more useful than the average? We need to do some thinking on the best way to handle this one... |
Add comment in the code explaining why it's better made inactive and also how to view as an instantaneous field in history
I came up with this for now (always open to modifying):
This way users must make a conscious choice about how they want to look at this field. |
Error checking: Update function avgflag_valid to stop the run when 'I' is used for history fields, unless specified in the namelist using hist_avgflag_pertape
Next steps:
|
Similar to previous most recent commit, which did the same for 'I'
Next topic: Existing time definitions in ctsm history and what I think we want instead. My current tests consist of 5-day simulations with daily tapes:
The last h* files (.h*.2000-01-06-00000.nc) all contain the following time definitions currently:
[...]
What I think we want to change: In all but 'I' and 'L' tapes, change the file name to .h*.2000-01-05-43200.nc and
to indicate the midpoint in the tape's time_bounds. In 'Lxxxxxx' tapes, change the file name to .h*.2000-01-05-sssss.nc and:
where sssss is the local time xxxxxx in seconds. In 'I' tapes, I think we change nothing. A possibly minor detail: Update for completeness...
And here from timestep output grouped into daily files (actually the last timestep, which ends up separate in the last file):
I will take notes in the upcoming meeting as to what we want the time variables to change to. |
@slevis-lmwg thank you very much for moving ahead with this! Sorry for my delayed reply (I was on vacation). Replying to a few points:
|
@cacraigucar @nusbaume @cecilehannay @billsacks @ekluzek I now wanted to more proactively draw attention to what I have done this far, so that we may work toward ensuring consistency across components before I invest additional time on this PR:
In the same vein of working toward ensuring consistency and efficiency, the post directly above this one discusses how I will change time definitions for the various history tapes. I will put this PR on hold until I have feedback on both the work completed and the work planned. Thanks, everyone. |
Thank you for emailing the LIWG group about this @billsacks. I changed the language for this field as you recommended. I also changed it for the few other instantaneous fields that were already inactive.
I think it will be most productive/efficient to have a short meeting to ensure that we're all on the same page. I will send an invite to @phillips-ad @strandwg @billsacks @ekluzek , permitting everyone to add more guests and to propose an alternate meeting time if my choice doesn't work.
This can go on the above meeting's agenda as a lower priority topic. |
From today's discussion with @slevis-lmwg @ekluzek @phillips-ad - The tentative plan is not to change the variables like mcdate, mcsec, etc. We'll check with CAM and check with users... but plan is to just document these better as being the model time at which the time sample is written. We had a lot of discussion on the subtle point of whether the time variable should give the average of time values in the center of the time step or at the end of the time step. This probably isn't super-important, but we would ideally like things to feel self-consistent in this respect; this may be most important for high-frequency output. @ekluzek and I initially felt like it was most appropriate to think of the time of history variables being associated with the end of the time step because the history accumulations are done at the end of the run loop. But @slevis-lmwg made some arguments that it could be better to think of variables as applying throughout the time step, and so correspond to a time in the middle of the time step. It may be that the former is most appropriate for states and the latter for fluxes. Our tentative decision was to make the time variable be the average of time values in the middle of the time step; it feels like the resulting time values will be most intuitive to users. This means that the time variable for a given averaging period is simply the mean of the end points of time_bounds for that averaging period, which keeps things simple. (If we were to specify time as the average of the times at the end of the time steps, there was some question of whether we would also want to change time_bounds... which started to feel messy.) But, given the above decision, this raised the question of what the time value should be for instantaneous fields. For consistency, we tentatively decided that the time value for instantaneous fields should also be in the middle of the given time step, which is a change (by half a time step) from the current time value for instantaneous fields. One rationale for this is thinking of the edge case of time-average fields where we ask for a time-average but are doing output every time step. The values in this case should be identical to those you would get from doing instantaneous output every time step, so it felt like the time values should be the same in both cases. (We don't expect that producing time-average fields every time step is a common use case, except for debugging, but we think that the consistency argument extends to longer averaging periods as well, even though it's harder to see intuitively.) That then raised the question of whether we should include time_bounds for instantaneous fields. With the original specification of time at the end of the time step, time_bounds seemed superfluous, but with the above plan of changing time to be the center of the time step, it felt to some of us that we should keep time_bounds for instantaneous fields, with the bounds being the start and end time of the given time step (and so the time value would be at the center of these bounds). (If I remember correctly, @slevis-lmwg and I were slightly in favor of having time_bounds on instantaneous fields and @phillips-ad was slightly opposed, feeling it would be confusing to have time_bounds for an instantaneous field; I'm not sure how @ekluzek felt.) We would like to run these tentative decisions by others in the CLM group as well as other components so we can be as consistent as possible across CESM. |
Summary from today's meeting with @phillips-ad @billsacks @ekluzek @slevis-lmwg
There was discussion whether to define history from a single timestep as the end or the middle of the timestep. We agreed to treat history from a single timestep as the middle of the timestep mainly for consistency when defining history from a time average. If we were to define history from a single timestep as the end of the timestep, then we would need Based on this decision and the fact that a single timestep instantaneous field is identical to a single timestep average field, we decided to make |
From the 2023/7/25 co-chairs' meeting, these are the group's modifications to our proposal:
For reference, these are the slides that I used to present our proposal at the co-chairs' meeting. |
Thanks @slevis-lmwg - yes, that agrees with my takeaways from this morning's meeting. As we discussed, I am comfortable with these changes. We also just talked about this in today's CSEG meeting. Brian Dobbins raised the question about whether we should put in place a temporary (to be removed soon) namelist flag that allows reverting to the original behavior in terms of the time value for non-instantaneous fields, so that if problems arise in the ongoing coupled model testing with having the new time value (e.g., for running diagnostics) we can temporarily revert this change. My feeling, which seemed to be shared by others, is that we go ahead with this change without namelist control, but then we can temporarily back it out if it turns out to be critical to do so. However, I don't feel strongly on this point, so would be open to having a temporary namelist flag if others want to go that route. We also discussed whether we should wait to bring in the CTSM changes until CAM is ready as well. The feeling is that we should not wait, but should go ahead as soon as CTSM is ready. This reminds me, though: @slevis-lmwg - are you or others also going to work on the similar changes in RTM (ESCOMP/RTM#32) and MOSART (ESCOMP/MOSART#52)? This discussion also made me think that it would be good to try running the LMWG diagnostics on this change to see if we are going to need changes to the LMWG diagnostics code to handle this change in time. |
I do not mind at all trying without a temporary namelist control. I will make the changes that we have discussed and then run our test-suites, as well as the LMWG diag. pkg (good suggestion). I'm happy to also work on the similar changes to RTM and MOSART. |
Testing the code mods with my own case I want to confirm whether I remember correctly that we would not modify the treatment of 'L' fields for now, other than placing in their own separate history tapes. |
Two of the updates move 'L' and 'I' history fields to separate history tapes as now required
This time 'I' fields are removed from time-average history tapes and not moved elsewhere for now
The LMWG diag. pkg completed successfully for a 3-yr run generated by one of the tests in the test-suite: |
Awesome! Have you confirmed that the output looks good? Specifically, I'm wondering if there might be some adjustments made in the LMWG package to shift time by 1/2 month or 1 month that should now be removed. |
Hmm, to answer your question I should compare against the same test when run outside this PR. |
I guess so... I'm not familiar enough with the LMWG diagnostics to give any useful input here. |
To address this question I used the diagnostic package to compare output from a test-suite test in the current PR The results show no differences between the two, suggesting that the diagnostic package does not shift the @olyson could you offer feedback whether the diagnostic package has code (that should now be removed) that reads the |
@slevis-lmwg @billsacks @ekluzek @cacraigucar There have been ongoing discussions within AMP on how to address the presence of instantaneous fields and accumulated fields on the same file (especially when there is no avgflag set for the tape). I know that you guys have changed all the 'I' flags in addfld calls to 'A', but there's concern that there are fields that don't make sense when averaged (and also we have A LOT of addfld calls that would need be changed). An alternate idea was proposed by @nusbaume : When a history tape has both instantaneous and accumulated fields on it (either through the default addfld calls or through ":" notation in the fincl list), two files are generated for that one "tape" - one for accumulated fields with the new midpoint time variable and one for the instantaneous fields with the existing time variable. For example, for the default history tape, you may end up with both h0a and h0i. What are people's thoughts on this? How much does this ruin everything and/or what caveats do we see going forward (with implementation, diagnostics, etc?) We may discuss this further at the 8/29 CSEG meeting, but I wanted to get the discussion started here before CAM and CTSM's implementations/thoughts diverge too much. |
Another feature with this proposition that our scientists liked is that existing fincl lists which contain a mixture of 'A', 'I', 'M' and 'X' would not need to be changed, but that they would be automatically split between the h0a and h0i files as needed. |
@peverwhee thanks for sharing these thoughts. It does sound like things are trickier on the CAM side, since in CTSM we had very few fields that were typically output as instantaneous. So you have the issue of how to deal with default-active fields, which we sidestepped. There was a lot of discussion about the naming of the different streams – avg vs instantaneous – in ESCOMP/CESM#194, though I haven't gotten my head back into that discussion enough to be able to summarize it here... and I'm not sure it ever actually came to a clear, summarizable conclusion. One feeling I do have – and which I think was a general feeling in that discussion – is that, if possible, we should try to keep CTSM and CAM consistent, so I appreciate your raising this so we can discuss it together. I don't see any major issues with your proposal for CAM. The idea of having different suffixes for average vs instantaneous files is something we also considered for CTSM; if I remember correctly, we didn't have strong feelings on whether to do that or not, so I think we could reconsider it for the sake of staying consistently with CAM if you are leaning strongly in that direction. The main thing I wonder about is whether users would expect to be able to control the frequency of instantaneous fields separately from time-averaged fields: since these end up on different history tapes, would it be confusing or feel limiting that you list both sets together in the namelist? I'm thinking that, once users get accustomed to seeing time-average vs instantaneous fields on different files, it might be more intuitive to think about controlling the field lists and time frequency of each one separately. But maybe not, and maybe this isn't a huge deal anyway. Other than that, the main thing I'm thinking about here is that your proposal would probably be somewhat harder to implement, at least on the CTSM side, so we'll need to discuss whether it's worth the time investment to do it this way. We should discuss this more at an upcoming CTSM meeting. |
Thanks for bringing this up @peverwhee. I think I like the idea of having A, L, X, M, S "averaged" files on ha files, and instantaneous on hi files. As @billsacks pointed out we decided against that, but we would reconsider to be consistent with CAM. A downside I see of the idea of making fields show up on both the ha files as A and hi files as I -- is that you double disk usage. I'd prefer making the user pick what they want, since we are in a world were management of disk space is a real issue. We already output a lot of fields already, and making it easier for users to unintentionally double their output doesn't sound good to me. And honestly the disk space usage is more of an issue with CAM than with CTSM. |
I just looked at the code and we only have about a dozen "I" fields, while CAM has somewhere around 700. So a much bigger issue in CAM than CTSM as @billsacks pointed out. And there's like 3800 calls to addfld, and CTSM has 2000 fewer. So it's clear to see why you don't want to be changing all of those calls, whether it's just the "I" fields or all of them. That's what @peverwhee pointed out to begin with. We do have some user-mod and test directories that specify lists of fields to add, but there's few enough of them that just changing them is workable for us. But @cacraigucar is right NOT having to change them is easier, especially for CTSM scientists. |
@peverwhee can correct me if I'm wrong, but my understanding is that fields would only show up on one of the two files: the user specification of fields would be the same as it currently is, and then at model runtime, fields would be divided into separate history tapes depending on whether they're being asked for as time-average or instantaneous. |
@billsacks is correct. Each field would only appear once between the two files. So there would be a net increase in disk usage, but not double. As for what Bill mentioned about it being more intuitive to for future users to put instantaneous and accumulated fields on different tapes, I agree. Our proposal definitely focuses more on handling existing configurations (which are aplenty in CAM), so I totally understand that it would result in further discussion on y'all's part because of the extra effort in implementing... |
I have very little to add:
|
Ahhh, OK rather than putting them on both, you just sort them into either the ha or hi file. Thanks for that correction. That sounds a lot better to me. |
I'm less concerned about disk space here, since everything gets converted to single variable history files for long-term storage. More concerning @olyson noted with would be change needed to modify post processing tools, time series generation, and diagnostics packages. It may be less problematic somewhat to maintain the file naming convention of clm2.h0. for the time averaged (max, min, sums) etc. and then a new file name for the instantaneous field (e.g., clm2.h0i.). |
@peverwhee @cacraigucar just to add to @wwieder 's latest comment: We talked about this in the ctsm-software meeting this morning. We are on board with the basic ideas you propose here and would like to stay consistent with CAM if the implementation doesn't end up being too difficult. Our thinking is to let CAM move ahead with these changes first so you can make sure this plan is going to work from your end, then assuming it does, we will probably adopt the same idea. As @wwieder mentioned, our preference would be to keep the file names the same as they currently are for the non-instantaneous fields. This is both to avoid needing to change the diagnostics package and other post-processing scripts and also because the 'a' (for 'average') in these file names is a bit misleading because they also contain various other fields (max over time, min over time, etc.). CAM folks: What is your feeling on this? |
@billsacks @wwieder Thanks for the feedback. I'll be starting to implement this (hopefully) tomorrow, so I'll let you guys know if any of our plans change. As for the filenames, I understand the hesitation to rename the 'A' files (I think technically 'a' would stand for 'accumulated', but that's not clear so it would likely still cause confusion). I don't have a problem with only adding the qualifier for instantaneous tapes, but I will bring it up at our next CAM meeting (Tuesday) and let y'all know the consensus. I do think that there will be some diagnostics updates needed regardless of the naming convention we settle on, but I agree that minimizing these changes is ideal. |
@peverwhee - ah, thanks for clarifying the meaning of "a". That does make more sense, then. We'll be interested to hear how strongly CAM folks feel about this name change. CTSM folks (@wwieder @slevis-lmwg @ekluzek @olyson and others) – if CAM folks feel fairly strongly that they want to make this change in file names, I wonder if we should take the opportunity to change our history output more significantly, changing "clm2" to "ctsm". It would be good to think about how much pain it would cause to do this rename – from h0 to h0a and/or from clm2 to ctsm. The latter is something that we've been talking about doing for a while and it seems like eventually we should bite the bullet and just do it, even though the timing never feels right... and if we're going to do that anyway, then could a rename from h0 to h0a come along for the ride if CAM folks feel pretty strongly that they want this on their end? |
One thing which I like about the total rename change (no h0 file anymore) is that it forces users to take a look at their files. Because instantaneous fields no longer would reside in an h0 file, I would like to force the users to address this specifically. They would need to make a name change as well as figuring out how to handle that they now may need to process two files. If the file is not renamed, they run the risk of thinking everything is working fine until they try to figure out why their instantaneous fields are no longer being plotted. Not a huge deal, but another minor point to consider. I don't feel strongly about this, but wanted to point it out. |
@cacraigucar that's a good point, and there is something more intuitive about having symmetrical names (h0a and h0i) vs. having what feels less symmetrical (h0 and h0i). Probably part of the difference in opinion between CAM and CTSM is that instantaneous fields are rare in CTSM (there are probably many runs where we don't output any instantaneous fields at all), whereas they appear more common in CAM. |
Nice discussion, thanks everyone especially @peverwhee and @cacraigucar . Yes, I agree with you @billsacks, if we are making changes that require changes to post-processing we might as well "upgrade" from clm2 to ctsm. We've been stuck on clm2 for over 2 decades now -- maybe it's time? ;-) |
Oh @ekluzek I love this idea, but was afraid to ask how hard it would be to use current the model name/version in our history files! I'm imagining this may create some challenges again with post processing and analysis workflows, but maybe these are pretty easy to work around? |
@billsacks idea. Changing clm2 to ctsm is one line in the code. There are a couple other places that we should change as well (like run_neon.py). But, a very small number. And since we already have to change for post processing scripts it seems like this would be easy to do at the same time. What would be difficult is changing this for the CTSM inputfiles as there are over 2600 references to "clm2" for inputdata files. We'd also have to copy all the datasets in inputdata archiving to the new lnd/ctsm directory from lnd/clm2. Doing that doesn't seem worth it right now... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked over ESCOMP/CAM#903 (i.e. the CAM equivalent of this PR) and reacquainted myself with my work in this PR (#2019).
I conclude that I should close this PR and start fresh, because only a few of my mods to histFileMod.F90 should be kept; the rest of the PR can be reverted. From the PR's list of commits, I identified the most relevant for the next PR and marked specific lines of code as "keep" or "revert":
4fc69a5 Error checking: Update function avgflag_valid to stop the run when 'I' is used for history fields, unless specified in the namelist using hist_avgflag_pertape
62c7d06 Upd. function avgflag_valid: abort when 'L' not in hist_avgflag_pertape
87a3558 If avgflag /= 'I', time_bounds is present and time = mid of time_bounds
7a52884 Clarify long_name for several history time variables
d22eca5 ChangLog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert changes to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert changes to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert changes to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this PR replicates CAM's approach to separating instantaneous from other fields, then I will likely not need some (all?) of the changes that appear in this and other user_nl_clm files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert changes to this file.
@@ -5930,7 +5962,8 @@ subroutine hist_do_disp (ntapes, hist_ntimes, hist_mfilt, if_stop, if_disphist, | |||
end subroutine hist_do_disp | |||
|
|||
!----------------------------------------------------------------------- | |||
function avgflag_valid(avgflag, blank_valid) result(valid) | |||
function avgflag_valid(avgflag, blank_valid, local_valid, & | |||
instantaneous_valid) result(valid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert.
@@ -5943,6 +5976,8 @@ function avgflag_valid(avgflag, blank_valid) result(valid) | |||
logical :: valid ! function result | |||
character(len=*), intent(in) :: avgflag | |||
logical, intent(in) :: blank_valid ! whether ' ' is a valid avgflag in this context | |||
logical, intent(in) :: local_valid ! is 'L' a valid avgflag or not | |||
logical, intent(in) :: instantaneous_valid ! is 'I' a valid avgflag or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert.
avgflag == 'SUM') then | ||
valid = .true. | ||
else if (avgflag(1:1) == 'L') then | ||
else if (avgflag(1:1) == 'L' .and. local_valid) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this section.
Better to start over but referring to this PR for a few changes. |
Also refer to ESCOMP/CAM#1003, as it fixes a bug with monthly averaged output that was in ESCOMP/CAM#903 |
Thank you @cacraigucar |
...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
Description of changes
Require instantaneous fields on separate history tapes.
Specific notes
Contributors other than yourself, if any:
@ekluzek @billsacks @olyson @samsrabin
CTSM Issues Fixed (include github issue #):
Fixes #1059
Are answers expected to change (and if so in what way)?
No, except:
As of now I have changed SNOW_PERSISTENCE from instantaneous to average.
Any User Interface Changes (namelist or namelist defaults changes)?
I plan to make the instantaneous option unavailable when adding history fields
'I'
will not be accepted; orfincl
namelist variable, so:I
will not be accepted.Instead users will add instantaneous variables with
fincl
namelist variable, but NOT using:I
andhist_avgflag_pertape
namelist variableExample of placing an instantaneous field in the second history tape.
Testing performed, if any:
This far I have performed a 5-day simulation with
In this test, I have not, yet, made the instantaneous option unavailable when adding history fields in the code with
'I'
or in the fincl syntax with:I
.NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).