ice_history: allow per-stream suffix for history filenames

dabail10 commented 8 months ago

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium, please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

[X] Short (1 sentence) summary of your PR: This is a potential solution to issue #854
[X] Developer(s): dabail10 (D. Bailey)
[X] Suggest PR reviewers from list in the column to the right.
[X] Please copy the PR test results link or provide a summary of testing completed below. Tested in CESM aux_cice.

https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#d05b1f30e57d2531799d4dbfb53464422582dd6f

How much do the PR code changes differ from the unmodified code?
- [X] bit for bit
- [ ] different at roundoff level
- [ ] more substantial
Does this PR create or have dependencies on Icepack or any other models?
- [ ] Yes
- [X] No
Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
- [ ] Yes
- [X] No
Does this PR add any new test cases?
- [ ] Yes
- [ ] No
Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
- [X] Yes
- [ ] No, does the documentation need to be updated at a later time?
  - [ ] Yes
  - [ ] No
[X] Please provide any additional information or relevant details below: This adds a new namelist parameter hist_str. This will allow for arbitrary extension on each of the history file streams. I am using 'x' to denote no extension. Happy to discuss this. For example in CESM we would use:

hist_str = 'x','1','2','3','4'

to get filenames cice.h, cice.h1, cice.h2, cice.h3, cice.h4.

DeniseWorthen commented 8 months ago

@dabail10 I'm confused about the situation where this would be used. What are h1,h2 etc?

Also, would this change the name of the history file---would iceh_06h become iceh_06hx ?

dabail10 commented 8 months ago

Good question. So, the code is in the subroutine construct_filename as follows. Basically, all this would do would be to change the 'cstream' variable. So, for instantaneous, the filenames would have the strings cice.h?_inst. Where history_file is set to something like 'ice.h'. For CESM, history_file is set to 'casename.cice.h'. You could potentially add a string like '_06h' if you wanted. Basically it is just meant to set 'cstream' uniquely for each history stream. Currently by default all of streams come out as iceh.YYYY-MM.nc or iceh.YYYY-MM-DD.nc, etc. in CICE standalone and casename.cice.h.YYYY-MM.nc and casename.cice.h.YYYY-MM-DD.nc, etc. in CESM. Does that make sense?

          if (histfreq(ns) == '1' .and. histfreq_n(ns) == 1)  then ! timestep
             write(ncfile,'(a,a,i4.4,a,i2.2,a,i2.2,a,i5.5,a,a)')  &
                   history_file(1:lenstr(history_file))//trim(cstream),'_inst.', &
                   iyear,'-',imonth,'-',iday,'-',msec,'.',trim(suffix)
          elseif (histfreq(ns) == '1' .and. histfreq_n(ns) > 1)  then ! timestep
             write(ncfile,'(a,a,i4.4,a,i2.2,a,i2.2,a,i5.5,a,a)')  &
                   history_file(1:lenstr(history_file))//trim(cstream),'.', &
                   iyear,'-',imonth,'-',iday,'-',msec,'.',trim(suffix)
          elseif (histfreq(ns) == 'h'.or.histfreq(ns) == 'H') then ! hourly
             write(ncfile,'(a,a,i2.2,a,i4.4,a,i2.2,a,i2.2,a,i5.5,a,a)')  &
                   history_file(1:lenstr(history_file))//trim(cstream),'_', &
                   histfreq_n(ns),'h.',iyear,'-',imonth,'-',iday,'-',msec,'.',trim(suffix)
          elseif (histfreq(ns) == 'd'.or.histfreq(ns) == 'D') then ! daily
             write(ncfile,'(a,a,i4.4,a,i2.2,a,i2.2,a,a)')  &
                   history_file(1:lenstr(history_file))//trim(cstream),'.', &
                   iyear,'-',imonth,'-',iday,'.',trim(suffix)
          elseif (histfreq(ns) == 'm'.or.histfreq(ns) == 'M') then ! monthly
             write(ncfile,'(a,a,i4.4,a,i2.2,a,a)')  &
                   history_file(1:lenstr(history_file))//trim(cstream),'.', &
                   iyear,'-',imonth,'.',trim(suffix)
          elseif (histfreq(ns) == 'y'.or.histfreq(ns) == 'Y') then ! yearly
             write(ncfile,'(a,a,i4.4,a,a)') &
                   history_file(1:lenstr(history_file))//trim(cstream),'.', &
                   iyear,'.',trim(suffix)
          endif

apcraig commented 8 months ago

This looks fine to me. It looks like "x" means there is no extension, nothing is added. I'm fine with that approach. It means you could not have "x" as an extension. The alternative, is to set the defaults to empty values ("") and then allow "x". Also, for all these history strings, I think if there is whitespace at the start or in the string, things will fail. I don't think we have those checks now, but they could be added. Not suggesting it's required though.

dabail10 commented 8 months ago

I thought about empty strings, but I wanted to be explicit similar to what is done for the histfreq variable. I agree leading whitespace is going to make this fail. Does trim remove leading whitespace?

apcraig commented 8 months ago

Trim does not remove leading whitespace.

apcraig commented 8 months ago

Please report CICE results when complete.

DeniseWorthen commented 8 months ago

As far as I know, we're not doing anything special to get a 06h in our file name. So I have

   histfreq       = 'm','d','h','x','x'
   histfreq_n     =  0 , 0 , 6 , 1 , 1
   history_file   = 'iceh'

which is producing iceh_06h.YYYY-MM-DD-SSSSS.nc. I'm concerned your change would produce iceh_06hx......

dabail10 commented 8 months ago

elseif (histfreq(ns) == 'h'.or.histfreq(ns) == 'H') then ! hourly write(ncfile,'(a,a,i2.2,a,i4.4,a,i2.2,a,i2.2,a,i5.5,a,a)') & history_file(1:lenstr(historyfile))//trim(cstream),'', & histfreq_n(ns),'h.',iyear,'-',imonth,'-',iday,'-',msec,'.',trim(suffix)

I see. So for the hourly I do see that the string _06h gets added for six-hourly output as follows:

elseif (histfreq(ns) == 'h'.or.histfreq(ns) == 'H') then ! hourly write(ncfile,'(a,a,i2.2,a,i4.4,a,i2.2,a,i2.2,a,i5.5,a,a)') & history_file(1:lenstr(historyfile))//trim(cstream),'', & histfreq_n(ns),'h.',iyear,'-',imonth,'-',iday,'-',msec,'.',trim(suffix)

However, with this change if you set that hist_str for that stream to 'x' it would not change the filename. If you set it to '1' say your filename would be:

iceh1_06h.YYYY-MM-DD-SSSSS.nc

Dave

DeniseWorthen commented 8 months ago

@dabail10 OK, thanks, I think that is fine then.

phil-blain commented 8 months ago

Also, I think

ice_history: allow per-stream suffix for history filenames

would be a more descriptive title for this PR.

dabail10 commented 7 months ago

@phil-blain I think I have addressed everything here. Did you have an opinion about using 'x' or '' for no suffix?

phil-blain commented 7 months ago

Regarding '' vs 'x': no, I don't have a strong preference for one or the other.

phil-blain commented 7 months ago

The only other thing I mentioned that you did not address is my suggestion here: https://github.com/CICE-Consortium/CICE/pull/912#pullrequestreview-1742868438 regarding having more than one frequency with hist_avg=.false. I think it's worth documenting.

dabail10 commented 7 months ago

The only other thing I mentioned that you did not address is my suggestion here: #912 (review) regarding having more than one frequency with hist_avg=.false. I think it's worth documenting.

Aha. I thought I had done that. Maybe we should put a separate fix in for this? I think this is related to how histfreq and f_var interact. For example, if you have histfreq = 'm','d','x','x','x' and f_aice = 'dxxxx' This will still output a daily stream. @eclare108213 added this as a feature awhile back. Or is it just related to the filenames?

phil-blain commented 7 months ago

What I'm talking about is this:

    histfreq       = 'd','h','x','x','x'
    histfreq_n     =  1 , 1 , 1 , 1 , 1
    hist_avg       = .false.,.false.,.true.,.true.,.true.
    f_aice         = 'd'
    f_hi           = 'h'

Since all instantaneous streams have the same filename, with the above settings we will have only hourly hi outputs, but no aice output (since the filenames are the same). By adding e.g. hist_suffix='daily','hourly' or something like this, we make the filenames different and can have separate sets of outputs.

But I can this doc doc change separately later, no problem.

dabail10 commented 7 months ago

What I'm talking about is this:
    histfreq       = 'd','h','x','x','x'
    histfreq_n     =  1 , 1 , 1 , 1 , 1
    hist_avg       = .false.,.false.,.true.,.true.,.true.
    f_aice         = 'd'
    f_hi           = 'h'
Since all instantaneous streams have the same filename, with the above settings we will have only hourly hi outputs, but no aice output (since the filenames are the same). By adding e.g. hist_suffix='daily','hourly' or something like this, we make the filenames different and can have separate sets of outputs.

But I can this doc doc change separately later, no problem.

I guess I still don't understand how this could happen? If one is daily and the other is hourly, then the two streams will have different file names, one with YYYY-MM-DD and the other with YYYY-MM-DD-SSSSS. Now, if you had two different hourly streams this might be a problem. I am actually thinking that we can add the label "_inst" for streams that have hist_avg = .true. This doesn't completely solve the problem you are mentioning. I think this is a bigger issue that we should fix.

phil-blain commented 7 months ago

If one is daily and the other is hourly, then the two streams will have different file names, one with YYYY-MM-DD and the other with YYYY-MM-DD-SSSSS

No, that's not what the code does. If a stream has .false. as its hist_avg value, then the filename is something like iceh_inst.YYYY-MM-DD-SSSSS.nc, this is the else branch here: https://github.com/CICE-Consortium/CICE/blob/f81b4c73858b0ce996bf3f06e403b33dc76332a8/cicecore/cicedyn/analysis/ice_history_shared.F90#L796-L799.

So, that's why in my example above, both streams end up with the same filename (they are both instantaneous). By setting hist_suffix to something different for the two streams, the filenames are distinct.

dabail10 commented 7 months ago

I totally missed that. That is a bug in my mind and I will create an issue.

apcraig commented 7 months ago

I think this is ready to merge? I'm going to run the io_suite on cheyenne just to check everything works fine. Then I'll merge unless someone pipes up.

dabail10 commented 7 months ago

I think this is ready to merge? I'm going to run the io_suite on cheyenne just to check everything works fine. Then I'll merge unless someone pipes up.

As long as you are happy with this, I would like it merged soon. I will work on the hist_avg thing separately.

apcraig commented 7 months ago

Testing looks good. Ran full test suite on cheyenne and io_suite on derecho with multiple compilers, all tests pass and bit-for-bit.

cheyenne:first_suite,base_suite,travis_suite,decomp_suite,reprosum_suite,io_suite,omp_suite,gridsys_suite,perf_suite,quick_suite,unittest_suite -m cheyenne -e intel,pgi,gnu

derecho:quick_suite,base_suite,io_suite -m derecho -e intel,cray,gnu,nvhpc,intelclassic

CICE-Consortium / CICE

ice_history: allow per-stream suffix for history filenames #912

PR checklist