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

cam6_4_049: History bugfixes #1163

Open
wants to merge 21 commits into
base: cam_development
Choose a base branch
from

Conversation

peverwhee
Copy link
Collaborator

@peverwhee peverwhee commented Oct 3, 2024

Summary

  1. Add optional sampled_on_subcycle argument to addfld. If that's true for a field, we don't override the averaging flag to "I" if nhtfrq = 1 (every timestep). This reenables subcycling averages.
  2. Check for incompatible duplicates
    • compatible duplicates (flags match) are ok - just take the first one (to keep existing CAM behavior)
    • incompatible duplicates (flags differ) - endrun
  3. Remove time_bounds field from instantaneous history files
  4. Update default filename for monthly instantaneous files to include full timestamp
  5. Update git-fleximod to 0.9.2
  6. Improve logging for cam_pio_openfile

Mods

src/control/cam_history.F90

  • Remove nhtfrq=1 override
    • Add sampled_on_subcycle logic to keep override for subcycle accumulated fields
  • modify list_index to optionally check for duplicates/compatibility
  • endrun with message including all duplicates found (if necessary)
    • Sample error message: ERROR: FLDLST: Found duplicate field(s) with different averaging flags. Place in separate fincl lists: "Q:A", "Q:I" (fincl3). "T:A", "T:I" (fincl3). "U:A", "U:I" (fincl4).
  • Remove time_bounds from instantaneous history files
  • Override instantaneous file name for monthly files to include full timestep

src/control/cam_history_support.F90

  • Add sampled_on_subcycle to field_info object

src/physics/*

  • update subcycle addfld calls to include sampled_on_subcycle=.true.

src/utils/cam_pio_utils.F90

  • Move filename logging to before pio call

bld/namelist_files/use_cases/sd_waccm_sulfur.xml

  • remove bad XML comment line

.lib/git-fleximod/*

  • update to 0.9.2

closes #1149
closes #1150
closes #1166
closes #1167
closes #1183

@peverwhee peverwhee self-assigned this Oct 3, 2024
@brian-eaton
Copy link
Collaborator

I'll just point out that the reason for setting the averaging flag to I when nhtfrq=1 is to get the correct timestamps. If the flag is A then the timestamps are set to midpoint values which is not correct for a single timestep.

@peverwhee peverwhee marked this pull request as ready for review October 29, 2024 19:16
@peverwhee
Copy link
Collaborator Author

@adamrher could you confirm that the addfld calls I've amended represent the full set of fields that are sampled on the subcycle?

Copy link

@adamrher adamrher left a comment

Choose a reason for hiding this comment

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

Hi @peverwhee, I found some missing addfld calls.

micro_pumas_cam.F90, marcop_driver.F90, microp_aero.F90 all look good. There are a 5 addflds you missed, in stats_init_clubb in clubb_intr.F90. I couldn't figure out how to add a review comment that deep into clubb_intr, but they start at line 5116.

microp_aero.F90 has some dependencies that have addfld calls. These are: ndrop.F90, ndrop_bam.F90, nucleate_ice_cam.F90, hetfrz_classnuc_cam.F90. All addflds in those modules need the sample_on_subcycle flag.

Lastly, the drivers in subcol_SILHS.F90 are called inside the macmic loop if use_subcol_microp=.true. Support for subcolumns is a grey area but it shouldn't be too much work (?) to add sample_on_subcycle to all the addflds in subcol_SILHS.F90.

src/physics/cam/check_energy.F90 Outdated Show resolved Hide resolved
@peverwhee
Copy link
Collaborator Author

@adamrher thanks for taking the time to check these! I've added/removed the sampled_on_subcycle arguments as you described.

  • stats_init_clubb in clubb_intr.F90 (5 fields)
  • ndrop.F90 (all addflds)
  • ndrop_bam.F90 (all addflds)
  • nucleate_ice_cam.F90 (all addflds)
  • hetfrz_classnuc_cam.F90 (all addflds)
  • subcol_SILHS.F90 (all addflds)
  • REMOVE FROM check_energy.F90 (all addflds)

Comment on lines 5740 to 5741
fname_inst = interpret_filename_spec( inst_filename_spec, number=(t-1), &
prev=prev, flag_spec='i' )
Copy link
Collaborator

Choose a reason for hiding this comment

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

A 1 month test reveals that the filename of an instantaneous monthly file still does not match the time data in the file. The problem is that monthly output files have prev=.true., so the timestamp in the filename is 1 timestep earlier than the time data in the file. Note that the prev option was implemented as a hack to get the expected month into the filename for monthly output files. The time at the end of a monthly period is at 0Z of day 1 of the following month. By backing up a timestep you get the date to contain the month that corresponds to the time period. This option should not be on for instantaneous data, but the logic that sets prev only takes into account the output frequency and not whether it's an instantaneous or accumulated file. I think the best solution here is to simply remove the prev=prev optional argument from this call to interpret_filename_spec since you never want to use a previous timestep for instantaneous data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, @brian-eaton !

I've removed the prev argument from interpret_filename_spec for the instantaneous file

Copy link
Collaborator

@brian-eaton brian-eaton left a comment

Choose a reason for hiding this comment

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

Looks good.

@peverwhee peverwhee changed the title History bugfixes cam6_4_047: History bugfixes Nov 11, 2024
@peverwhee peverwhee changed the title cam6_4_047: History bugfixes cam6_4_048: History bugfixes Nov 12, 2024
@peverwhee peverwhee changed the title cam6_4_048: History bugfixes cam6_4_049: History bugfixes Nov 13, 2024
Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

Curiosity led me to looking into this PR. This is not intended to be a full review, but rather documents a couple of things that I saw.

@@ -2,7 +2,7 @@
import argparse
from git_fleximod import utils

__version__ = "0.8.4"
__version__ = "0.9.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that CESM is at 0.9.3 - should we be updating to that while we are at it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep! i will do this tomorrow!

@@ -68,6 +68,7 @@ case $hostname in
test_file_list="tests_pretag_izumi_${CAM_FC,,}"
cam_tag=$1_${CAM_FC,,}
baselinedir="/fs/cgd/csm/models/atm/cam/pretag_bl/$cam_tag"
chmod_cmd="chmod -R a+r ${baselinedir}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious what this change is doing and why it is needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

some cime changes recently resulted in the baselines on izumi having the wrong permissions. this updates the baselines to be globally readable.

per jesse's suggestion, I will be updating this to also run on derecho since it can't hurt to be globally readable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants