search

ai2cm / fv3config

Manipulate FV3GFS run directories
Apache License 2.0
1 stars 0 forks source link

Fix `diag_table` bug preventing reproducible restarts #144

Closed spencerkclark closed 2 years ago

spencerkclark commented 2 years ago

This PR introduces a get_time_configuration function in place of the existing get_current_date function. The get_time_configuration function returns both the initialization date and the current date.

The initialization and current dates can be found in lines two and three of any coupler.res restart file, and are read in accordingly when one is present in a set of restart files provided as an initial condition. If the initial condition does not contain a coupler.res file, or coupler_nml.force_date_from_namelist is set to true, the initialization date and current date are set to the value of the coupler_nml.current_date namelist parameter.

As found in #147, for reproducible restarts, it is critical that the base date written in the diagnostics table is the initialization date of the run (therefore it will remain constant across segments). Previously in segmented runs, we reset the base date to the date of the current segment, which prevented reproducible restarts. This PR fixes this issue (see this notebook).

Resolves #147 See also: ai2cm/fv3gfs-fortran#269

spencerkclark commented 2 years ago

I'm having some trouble following this code. Do you have some time to VC about it?

Sure. Most of the code/docstrings are adapted from what existed before, but I can rewrite it in a different way.

nbren12 commented 2 years ago

@spencerkclark Thanks for chatting. I dismissed the comments related to what we discussed. This is my understanding of what we settled on:

  1. Write a function get_timespec (?), with a signature like this get_timespec(config) -> (base_time, current_date)
  2. replace all usages of get_diag_table_base_date with get_timespec(config)[0]
  3. delete the unused helpers and new APIs get_diag_table_base_date, _get_initialization_date_from_coupler_res, _get_current_date_from_coupler_res

Food for later thought, it would be nice to load the information in coupler.res into the config at an earlier stage so that I/O is not needed to figure out the date from a loaded config.

spencerkclark commented 2 years ago

Thanks for chatting yesterday @nbren12. I think I addressed everything we discussed.

Food for later thought, it would be nice to load the information in coupler.res into the config at an earlier stage so that I/O is not needed to figure out the date from a loaded config.

Yes, this would be nice. The coupler_nml.current_date parameter might actually already encode the base date information. I think it needs to be set on initialization, but is left untouched in subsequent segments, and so would be propagated forward with the config. It might be a little confusing to rely on that assumption though.