ctsm5.3.018: Change history time to be equal to the middle of the time bounds #2838
Conversation
...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
…e_mid_of_time_bounds
slevis-lmwg
added
enhancement
|
I submitted this manual test to confirm that the committed modifications work as intended: |
|
The previous test completed its 1 month and the monthly output looked good, but there were annual history files that I could not tell. So I started another test (default is Ly1, but I changed to Ly2) and I added hist_avgflag_pertape(6) = 'I' to see what happens: PASS |
|
I updated the submodules to point to ESCOMP/MOSART#106 and ESCOMP/RTM#39 and submitted the three corresponding test-suites: All the cases that differ from the baseline, differ only in the time variable. UPDATE |
slevis-lmwg
added
the
external
slevis-lmwg
added
PR status: ready
time in hist now equals the middle of the time_bounds MOSART equivalent to CTSM work done in ESCOMP/CTSM#2838 Answers change only for the time variable. slevis resolved conflicts: src/riverroute/RtmHistFile.F90 src/riverroute/RtmTimeManager.F90
time in hist now equals the middle of the time_bounds RTM equivalent to CTSM work done in ESCOMP/CTSM#2838 Answers change only for the time variable.
|
@ekluzek review and approval of this PR should take 5 minutes, as it looks the same as the corresponding RTM and MOSART PRs that you reviewed/approved. Thanks :-) |
|
TODOs left for me:
|
|
izumi testing derecho testing Also I'm getting diffs in the cpl and mosart output of two tests BUT both are 3-yr tests: |
|
The diffs of the two tests above seem vaguely related to the earlier update to mosart1.1.02 (ESCOMP/MOSART#94), but Adrianna's test passed just fine pointing to mosart1.1.02. So I will try the two tests pointing to mosart1.1.02 and mosart1.1.03:
The same test from ctsm5.3.012: PASS DID THESE TESTS EXIST WHEN I LAST RAN aux_clm? Yes (ctsm5.3.009). So now I checked out 1e81456 from above, pointed to mosart1.1.04/rtm1_0_82, and submitted: |
|
I brainstormed for a bit with @billsacks and Bill pointed out/suggested:
|
|
@billsacks I mentioned to you a vague memory I had of an issue that could relate to these diffs, and it is this one: |
|
I'm wondering if the "treat a file as instantaneous if its first variable is" might be premature to bring in here instead of #2445. Specifically, I think the |
@olyson what do you think about @samsrabin's comment? Most concerning to me would be any vulnerability in the land diagnostic package. |
|
I don't think the land diagnostics package uses time_bounds. But I may not understand the issue here. Seems like we could do a short simulation using this branch once stable and see if there are any problems? Also should check ILAMB. So maybe an I2000 case for a test. |
|
@olyson Okay, good that the land diagnostics package probably doesn't use it, but yes would be nice to check. However, I'm thinking that other people's scripts might rely on the presence of |
|
And actually, this comment goes for the "exact middle" vs. "end of" change as well. It seems arbitrary (and against the "principle of least astonishment") that the first variable in the |
|
A bonus from what I'm proposing: Always (a) including |
|
The standard diagnostics package doesn't use time_bounds as far as I can tell. ILAMB may, it just needs to be tested. Always including time_bounds sounds fine to me. |
|
time_bounds is an expected part of the CF convention, so I do endorse using it for anything with time. That's likely why some tools might expect it to be there. For instantaneous it should likely be the time bounds of the time-step that was output. So the previous time-step time first to the ending time-step time for the endpoint. You could have both being the same ending time-step, but that doesn't show that it is a model with a finite time-step. Here's information on the CF Convention attributes. Look up "bounds"... |
|
@ekluzek That link contains the following text, which implies that instantaneous files should actually not have
|
|
Good point @samsrabin. That convinces me we should remove it for I fields then. That would then be in line with the convention. We don't similarly report on the grid cell bounds either (which could be done for 2D grids, but would be harder for unstructured grids), so we shouldn't for Instantaneous time fields either. |
|
Earlier discussion led to the conclusion that we should not have time_bounds in instantaneous files (though CAM's mistakenly still does): ESCOMP/CAM#1166 |
|
I had a quick meeting with @samsrabin 40 minutes ago: Oh, good, looks as though we're removing time_bounds from instantaneous tapes, as originally planned :-) Ok, so I'm testing the alternate if-statement with aux_clm right now and then I will push it to the PR.
How I'm checking whether diffs are expected: OK aux_clm except failure reported in the following post. |
|
@samsrabin back to this test |
|
@samsrabin I'm thinking please hold regarding my last post for now, because in my aux_clm tests with the two subsequent PRs, which included this one, the test passed. |
samsrabin
removed
the
PR status: ready
|
Note to @ekluzek
Note to self: |
Merge tmp-241219 branch to master Includes three tmp-241219 tags: tmp-241219.n01.ctsm5.3.016 Merge b4b-dev: nfix_method options Houlton (default), Bytnerowicz (option) tmp-241219.n02.ctsm5.3.016 FATES hydro test update tmp-241219.n03.ctsm5.3.016 Bug fix for izumi nag tests to pass (b4b unless using Bytnerowicz) Fixes ESCOMP#2924 Fix problem with izumi nag tests Fixes ESCOMP#2878 Remove fates_allom_smode shell_command update in FatesColdHydro testmod Fixes ESCOMP#2869 Update temperature cost function for symbiotic nfix in FUN Changes answers as documented in the ChangeLog. slevis resolved conflicts: doc/ChangeLog doc/ChangeSum
|
Updated to rtm1_0_84 and mosart1.1.06 and started on derecho: Mosart nvhpc test not labeled EXPECTED but has failed since mosart1.1.03-ctsm5.3.009 from what I can tell. How I check for aux_clm results in tests_0109-170044de:
...but why would the same test end up different like this?
On izumi Notes |
There was a problem hiding this comment.
@slevis-lmwg and I went over this. I had some comments and questions that we worked through. The testing is done so this can be made shortly.
The one suggestion I had was to point out in the ChangeLog that the mosart tag used for the Izumi baseline included the bug-fix that will be made in mosart1.1.07 that will come in the next tag ctsm5.3.019.
Since, testing and baselines had been done, and there are things that have to be done by hand on Izumi we didn't want to have to redo any of the testing.
ekluzek
changed the title
Description of changes
This PR subsets the scope of issue #1059 and PR #2445 as a result of the October 2024 conversation in #2445.
This PR changes history time to be equal to the middle of the time bounds.
This PR does not put instantaneous fields on their own separate history files.
I will also bring submodule changes from ESCOMP/MOSART#106 (was ESCOMP/MOSART#69) and ESCOMP/RTM#39.
Specific notes
Contributors other than yourself, if any:
Are answers expected to change (and if so in what way)?
No.
Does this create a need to change or add documentation? Did you do so?
Maybe. No.
Testing performed, if any:
Plan to run aux_clm, mosart, rtm test-suites.