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

fix the output file name and diag time #404

Merged
merged 85 commits into from
Oct 1, 2021

Conversation

junwang-noaa
Copy link
Collaborator

Description

The output file name does not have correct time stamp when the output time is not a integer. E.g. output_fh=0.25, the file name is currently atmf00.nc and sfcf00.nc. Also for random output time, the diag_time may not be updated when the output time is not multiple of fhzero. This PR contains fixes for the two issues.

Issue(s) addressed

Link the issues to be closed with this PR, whether in this repository, or in another repository.
(Remember, issues should always be created before starting work on a PR branch!)

Testing

How were these changes tested? Tested on hera.
What compilers / HPCs was it tested with?
Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
Have the ufs-weather-model regression test been run? On what platform?
Yes on hera.

  • Will the code updates change regression test baseline? If yes, why? Please show the baseline directory below.
  • No

Dependencies

If testing this branch requires non-default branches in other repositories, list them.
Those branches should have matching names (ideally)

ufs-weather-model PR
fv3atm PR

DeniseWorthen and others added 30 commits November 18, 2019 11:11
The HAFS related developments for the write_grid_component (NOAA-EMC#10)
change ifmin to zero at fh00 for inline POST (NOAA-EMC#14)
Update EMC dev/emc from NCAR dtc/develop 2019/12/17 (NOAA-EMC#30)
Regain bit-for-bit identical results between IPD and CCPP for coupled…
Merge NCAR:dtc/develop into develop 2020/04/14 (NOAA-EMC#98)
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

I don't understand the logic, especially in lines 763 to 772. Is it possible to document that code? Just a brief one liner what each of those blocks does would be great.

@DusanJovic-NOAA
Copy link
Collaborator

Why do need all this logic with lflname_fulltime, why can we just switch to using the filename format that always include HH-MM-SS, which will simplify the code substantially. Scripting will be also simplified by always using the same file naming scheme, that does not change based on the value output_fh.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Sep 30, 2021

Why do need all this logic with lflname_fulltime, why can we just switch to using the filename format that always include HH-MM-SS, which will simplify the code substantially. Scripting will be also simplified by always using the same file naming scheme, that does not change based on the value output_fh.

This is for backward compatible. Otherwise all the downstream jobs will be broken.

@DusanJovic-NOAA
Copy link
Collaborator

Why do need all this logic with lflname_fulltime, why can we just switch to using the filename format that always include HH-MM-SS, which will simplify the code substantially. Scripting will be also simplified by always using the same file naming scheme, that does not change based on the value output_fh.

This is for backward compatible. Otherwise all the downstream jobs will be broken.

Can't we announce the change and downstream jobs can update scripts. This is after all develop branch, we should be able to make breaking changes. Are we going to keep backward compatibility forever. Every change we make in any of the config file structure or output is potential breaking change for somebody.

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Thanks for adding those comments. A few suggestions/corrections of typos, otherwise looks good.

fv3_cap.F90 Outdated
@@ -746,6 +746,9 @@ subroutine InitializeAdvertise(gcomp, rc)
endif
do i=2,nfh
output_fh(i) = (i-1)*outputfh2(1) + output_startfh
! Except fh000, which is the first time output, if any other of the
! output time is not integer hour, set lflname_fulltime to be ture, so the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
! output time is not integer hour, set lflname_fulltime to be ture, so the
! output time is not integer hour, set lflname_fulltime to be true, so the

fv3_cap.F90 Outdated
@@ -760,6 +763,10 @@ subroutine InitializeAdvertise(gcomp, rc)
count=noutput_fh, rc=rc)
if (ESMF_LogFoundError(rcToCheck=rc, msg=ESMF_LOGERR_PASSTHRU, line=__LINE__, file=__FILE__)) return
if( output_startfh == 0) then
! if the output time in output_fh array contains first time stamp output,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
! if the output time in output_fh array contains first time stamp output,
! If the output time in output_fh array contains first time stamp output,

fv3_cap.F90 Outdated
@@ -760,6 +763,10 @@ subroutine InitializeAdvertise(gcomp, rc)
count=noutput_fh, rc=rc)
if (ESMF_LogFoundError(rcToCheck=rc, msg=ESMF_LOGERR_PASSTHRU, line=__LINE__, file=__FILE__)) return
if( output_startfh == 0) then
! if the output time in output_fh array contains first time stamp output,
! check the rest of output time, otherwise, check all the output time.
! if any of them is not integer hour, the history file names will
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
! if any of them is not integer hour, the history file names will
! If any of them is not integer hour, the history file names will

fv3_cap.F90 Outdated
@@ -773,6 +780,9 @@ subroutine InitializeAdvertise(gcomp, rc)
else
do i=1,noutput_fh
output_fh(i) = output_startfh + output_fh(i)
! when output_startfh >0, check all the output time, if any of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
! when output_startfh >0, check all the output time, if any of
! When output_startfh >0, check all the output time, if any of

fv3_cap.F90 Outdated
@@ -773,6 +780,9 @@ subroutine InitializeAdvertise(gcomp, rc)
else
do i=1,noutput_fh
output_fh(i) = output_startfh + output_fh(i)
! when output_startfh >0, check all the output time, if any of
! them is not integer hour, set lflname_fulltime to be ture. The
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
! them is not integer hour, set lflname_fulltime to be ture. The
! them is not integer hour, set lflname_fulltime to be true. The

@junwang-noaa
Copy link
Collaborator Author

@climbfuji I added some comments to explain, please check. Basically for frequency output, we will don't count the the first time step output as it is labeled as fh000.

@climbfuji
Copy link
Collaborator

@climbfuji I added some comments to explain, please check. Basically for frequency output, we will don't count the the first time step output as it is labeled as fh000.

I did, please look at/accept my suggestions.

@junwang-noaa
Copy link
Collaborator Author

junwang-noaa commented Sep 30, 2021

@DusanJovic-NOAA Well, this will impact future operational implementations. As we don't know if we will have higher frequency output, I think we may need to get the downstream involved before we make the hard code change. Right now if the output time is non-integer (needs min/sec info), the output file will have full time stamp, otherwise if it's all integer hours, file names will only contain hours.

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

Successfully merging this pull request may close these issues.

4 participants