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

Add flexible timining feature in modern diag_manager #1077

Merged
merged 16 commits into from
Dec 16, 2022

Conversation

uramirez8707
Copy link
Contributor

@uramirez8707 uramirez8707 commented Nov 23, 2022

Description

  1. Fixes need flexible output average capability #956 Adds use_clock_average namelist to diag_manager_nml
    If .true. the averaging is done based on the clock. For example, if doing daily averages and your start the simulation in day1_hour3, it will do the average between day1_hour3 to day2_hour 0. The default behavior will do the average between day1 hour3 to day2 hour3

  2. Adds the capability to allow for different file frequencies for the same file.
    For example, for this diag_file yaml entry

- file_name: flexible_timing%4yr%2mo%2dy%2hr
  freq: 1 1 1
  freq_units: hours hours hours
  time_units: hours
  unlimdim: time
  new_file_freq: 6 3 1
  new_file_freq_units: hours hours hours
  start_time: 2 1 1 0 0 0
  file_duration: 12 3 9
  file_duration_units: hours hours hours
  varlist:
  - module: ocn_mod
    var_name: var1
    reduction: average
    kind: r4

It will create a file every 6 hours for 12 hours
flexible_timing_0002_01_01_01.nc - using data from hour 0 to hour 6
flexible_timing_0002_01_01_07.nc - using data from hour 6 to hour 12

Then it will create a file every 3 hours for 3 hours
flexible_timing_0002_01_01_13.nc - using data from hour 12 to hour 15

Then it will create a file every 1 hour for 9 hours.
flexible_timing_0002_01_01_16.nc - using data from hour 15 to hour 16
flexible_timing_0002_01_01_17.nc - using data from hour 17 to hour 18
etc
etc

The filenames still need some more work.

  1. Adds code to write the time averaging relating variables (time_bounds, T1, T2, DT)
  2. Adds test to test these new features

How Has This Been Tested?
CI including added tests

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

@uramirez8707 uramirez8707 changed the title Flexible timinig Add flexible timining feature in modern diag_manager Nov 23, 2022
character(len=*) , intent(in) :: long_name !< The long_name of the variable
character(len=*) , intent(in) :: units !< The units of the variable

!TODO harcodded double
Copy link
Member

Choose a reason for hiding this comment

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

Is this for reproducibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hardcoded the double to make things easy for me.

I think in the old diag manager if you compile with r8 all of the time related variables were r8 and if you compile with r4 all of the time related variables were r4.

Copy link
Member

Choose a reason for hiding this comment

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

I see there is a TODO for this, so we can move on. We are supporting all types, and netcdf files can have all types in them.

Comment on lines 879 to 888
!< Register the "nv" dimension that it needed for the time_bounds
call register_axis(fileobj, "nv", 2)
!TODO hardcodded double
call register_field(fileobj, "nv", "double", (/"nv"/))
call register_variable_attribute(fileobj, "nv", "long_name", "vertex number", str_len=13)

!< Register the "time_bounds" as a variable
dimensions(1) = "nv"
dimensions(2) = trim(time_var_name)
bounds_name = trim(time_var_name)//"_bounds"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can think of something better than just repeating this

@uramirez8707 uramirez8707 marked this pull request as ready for review December 2, 2022 19:35
Copy link
Member

@thomas-robinson thomas-robinson left a comment

Choose a reason for hiding this comment

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

Combine the repetitive subroutines.

Add a namelist check for your new variable to make sure it's running with the modern diag

class(fmsDiagFileContainer_type), intent(in), target :: this !< The file object
TYPE(time_type), intent(in) :: time_step !< Current model step time
subroutine write_time_data(this)
class(fmsDiagFileContainer_type), intent(in), target :: this !< The file object
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the file object. This is the diag file object container. Why does this have to take the whole container and not just the diag file object?

Copy link
Member

Choose a reason for hiding this comment

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

It's only 1 container object being sent in, not the whole thing.

character(len=*) , intent(in) :: long_name !< The long_name of the variable
character(len=*) , intent(in) :: units !< The units of the variable

!TODO harcodded double
Copy link
Member

Choose a reason for hiding this comment

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

I see there is a TODO for this, so we can move on. We are supporting all types, and netcdf files can have all types in them.

domain => null()
diag_file => null()
end subroutine open_diag_file

!< @brief Writes the time average variables (*_T1, *_T2, *_DT) in the netcdf file
subroutine write_avg_time_metadata(fileobj, variable_name, dimensions, long_name, units)
Copy link
Member

Choose a reason for hiding this comment

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

Please combine the repetitive routines for maintainability

@jiandewang
Copy link

@uramirez8707 can we use this feature using diag_table ? is yes can you provide a sample ? thanks

@uramirez8707
Copy link
Contributor Author

@jiandewang unfortunately this feature is only going to be available for diag_table.yaml and the new diag_manager, which won't be production for at least a couple of months.

@jiandewang
Copy link

@jiandewang unfortunately this feature is only going to be available for diag_table.yaml and the new diag_manager, which won't be production for at least a couple of months.

@uramirez8707 thanks for the heads up

@jiandewang
Copy link

jiandewang commented Sep 14, 2023

@uramirez8707 can you tell me whether diag_table.yaml and the new diag_manager is available in FMS now ? I need to use diag_yaml to control the output frequency. I checked the commit log in current FMS but can't find b0334fc commit

@thomas-robinson
Copy link
Member

@uramirez8707 can you tell me whether diag_table.yaml and the new diag_manager is available in FMS now ? I need to use diag_yaml to control the output frequency. I checked the commit log in current FMS but can't find b0334fc commit

@jiandewang The updated diag manager is almost ready. We are coding the updated reduction routines which will be the last step before testing can begin. The reduction routines are needed so that the diag manager can be run through. The time frame we are looking at is October/November. When it's ready for testing, we would appreciate the help testing the flexible timing feature because we at GFDL don't have a use case and it appears you do.

@jiandewang
Copy link

@thomas-robinson thank you very much for the headup. Please let me know when it's ready. In EMC we are looking for this feature in our coupled system end-to-end global-worlkflow due to special requirement from DA group

@jiandewang
Copy link

@thomas-robinson can you tell me whether the new diag_manage and diag_table.yaml feature is in FMA main repo. ?

@thomas-robinson
Copy link
Member

@jiandewang we are currently testing the tag 2024.01-beta5 which is on the dmUpdate branch. If the tests all pass (which it seems like they finally will), we will be merging the code into main and releasing it at the end of this week as 2024.01. There is likely to be a follow on patch next week or the week after.

rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024
rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024
rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024
rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024
rem1776 pushed a commit to rem1776/FMS that referenced this pull request May 1, 2024
@jiandewang
Copy link

@thomas-robinson I just started my first try but without success.
The FMS I am using is cloned from main couple of days ago:
commit 42f8506 (HEAD -> main, tag: 2024.01.01-beta1, origin/main, origin/HEAD)
Author: Alex Huth huthalexandere@gmail.com
Date: Thu May 9 13:33:56 2024 -0400

for the ocean model I tested, using the following original diag_table works fine:
GOLD_SIS
1 1 1 0 0 0
#output files
"ocean", 3, "hours", 1, "hours", "time",
"ocean_model", "geolon", "geolon", "ocean", "all", .false., "none", 2
"ocean_model", "SSH", "SSH", "ocean","all",.false.,"none",2
"ocean_model", "SST", "SST", "ocean","all",.false.,"none",2

now I switched to diag_table.yaml but failed:
title: GOLD_SIS
base_date: 1 1 1 0 0 0
diag_files:

  • file_name: ocean
    time_units: hours
    unlimdim: time
    is_ocean: true
    freq: 3 hours
    varlist:
    • module: ocean_model
      var_name: geolon
      reduction: none
      kind: r4
    • module: ocean_model
      var_name: SSH
      reduction: none
      kind: r4
    • module: ocean_model
      var_name: SST
      reduction: none
      kind: r4

error message I got:
diag_manager_mod::diag_manager_init: Error parsing diag_table. Error reading the base date from the diagnostic table.

what's wrong in my yaml file ? Is there a namelist/flag to tell the system that we are using yaml style instead of traditioanl style ?

@uramirez8707
Copy link
Contributor Author

I think you are missing setting the use_modern_diag namelist to .True.
https://github.com/NOAA-GFDL/FMS/blob/main/diag_manager/diag_data.F90#L391

@jiandewang
Copy link

I think you are missing setting the use_modern_diag namelist to .True. https://github.com/NOAA-GFDL/FMS/blob/main/diag_manager/diag_data.F90#L391

thanks @uramirez8707. Now I got "You must compile with -Duse_yaml to use the option use_modern_diag". Let me recompile it and try again

@jiandewang
Copy link

jiandewang commented May 15, 2024

I add "Duse_yaml" to compile FMS and that worked fine. But when I tried to compile MOM6 I got error:
parser/yaml_output_functions.c:372: undefined reference to yaml_emitter_initialize parser/yaml_output_functions.c:373: undefined reference to yaml_emitter_set_output_file

do I need to also add "Duse_yaml" to compile MOM6 ?

@uramirez8707
Copy link
Contributor Author

Are you compiling FMS with cmake?

You shouldn't need to add that MOM6.

@jiandewang
Copy link

Are you compiling FMS with cmake?

You shouldn't need to add that MOM6.

no, I followed the compiling steps from MOM6-examples. See https://github.com/NOAA-GFDL/MOM6-examples/wiki/Getting-started#compiling-the-models

@uramirez8707
Copy link
Contributor Author

What system are you using?

You do need to install libyaml: https://github.com/yaml/libyaml
and include the -I include directory: https://github.com/NOAA-GFDL/mkmf/blob/master/templates/ncrc5-intel-classic.mk#L51 and the -L library directory and -llibyaml https://github.com/NOAA-GFDL/mkmf/blob/master/templates/ncrc5-intel-classic.mk#L146

@jiandewang
Copy link

I am using intel. Let me try to follow your information. Thanks @uramirez8707

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.

3 participants