Change from K[dv]_turb to K[dv]_shear in MOM restart (answer changer)

[wfcooke]

In a series of updates culminating with 80c33a4 Kd_turb was changed to Kd_shear.
The issue is that restart fields created prior to this update obviously contain Kd_turb and Kv_turb which are not read in with the new code resulting in a change of answers.

Users need to be aware that the Kd_turb and Kv_turb variables need to be renamed in their MOM restart file (and that can potentially be MOM_res_?.nc) if the file was produced prior to this update.

[adcroft]

Indeed, this name change (in the restart file) should not have slipped through with out being evaluated. This happened when the alternate version of the diabatic mode was added. We now have to decide whether to change it back or add logic to rename the variable depending on which mode is being used...

[adcroft]

@wfcooke Is it the case that you are renaming the variable in the restart file to work around this isseue? We are currently discussing this on the dev call and the complexity of solving this problem is raising the question of whether it's worth solving?

[marshallward]

I ran this past @russfiedler who pointed out there is some precedent for doing this in MOM5, specifically using field_exist to conditionally call restore_state.

       ! determine whether to read density derivs at the initial time step of integration.
       ! early versions of MOM4p1 did not contain drhodT and drhodS in the restart file, so 
       ! to allow older restart files to be used with newer releases of MOM4p1, we 
       ! provide for the possibility that the restarts do not contain drhodT and drhodS.
       if(field_exist(filename, 'drhodT')) then
           call restore_state( Den_restart, id_restart(3) )
           call restore_state( Den_restart, id_restart(4) )
           call mpp_update_domains(Dens%drhodT(:,:,:),Dom%domain2d)
           call mpp_update_domains(Dens%drhodS(:,:,:),Dom%domain2d)
       else 
           write(stdoutunit,'(a)') '==>Note: ocean_density_mod: did not read density derivatives from restart.' 
       endif 

       ! determine whether to read rho_salinity at the initial time step of
       ! integration.  Early versions of MOM4p1 did not contain the rho_salinity
       ! in the restart file.  This allows older restart files to be used with
       ! newer releases of MOM.
       if(field_exist(filename, 'rho_salinity')) then
          write (stdoutunit,'(/a)') '  Initializing salinity for use in density calculation from restart'
          call restore_state( Den_restart, id_restart_rho_s )
          Dens%rho_salinity(:,:,:,taum1) = Dens%rho_salinity(:,:,:,taup1)
          Dens%rho_salinity(:,:,:,tau) = Dens%rho_salinity(:,:,:,taup1)
          call mpp_update_domains(Dens%rho_salinity(:,:,:,taum1),Dom%domain2d)
          call mpp_update_domains(Dens%rho_salinity(:,:,:,tau),  Dom%domain2d)
          call mpp_update_domains(Dens%rho_salinity(:,:,:,taup1),Dom%domain2d)
       else
          write (stdoutunit,'(/a)') '  Initialising salinity for use in density calculation'
          call update_ocean_density_salinity(T_prog,taum1,Dens)
          call update_ocean_density_salinity(T_prog,tau,Dens)
          call update_ocean_density_salinity(T_prog,taup1,Dens)
       endif

https://github.com/mom-ocean/MOM5/blob/master/src/mom5/ocean_core/ocean_density.F90#L1123-L1153

[wfcooke]

@adcroft I have not modified anything yet, but something along the lines of what @marshallward mentioned would be a good backward compatibility update (imo)

[ashao]

Hi @marshallward, thanks for looking that up. It is interesting that there is some precedent and I wonder if @StephenGriffies can shed some light on the thought process. While I agree from a broad perspective that it's a nice-to-have feature to be able to maintain backwards compatibility vis-a-vis restarts, I do worry about how sustainable that is going forward given that the information required in a restart file may change over time.

I personally think about restarts as a way of just ensuring that a given model version can be guaranteed to be bit-for-bit identical no matter the time segments of integration. I don't think that we do, can, or should enforce that we can maintain bit-for-bit identity between model versions. From my point of view then, the main reason why you would want to use a restart from an older version of the model is to have a reasonable initial condition of tracers (both active and passive) and some elements of the dynamic state from a previous long integration. As @Hallberg-NOAA mentioned yesterday something like this capability already exists within the existing MOM6 code base.

I would propose that as part of an intermediate solution between implementing backwards compatibility for restart, we do a better job going forward to keep track of which restart variables are changed/dropped between tags of MOM6 and to communicate them as part of release notes. I do also like @adcroft 's proposal from yesterday to keep track of a list of deprecated restart variables and to throw a FATAL error if those names are detected in the restart file. As part of the solution as well, perhaps we can identify aspects of initialization process where it's unclear how to initialize variables from a restart-like file. Something like the user being able to provide a dictionary that maps variable names in an initial condition file to native model names.

[wfcooke]

@ashao when you say an older version of the model, do you mean an older tag of MOM6 or MOM4/MOM5? This issue is about a change in the MOM6 code that broke restarts. i.e. a tag from January did not get reproduced by a tag from August (or somewhere in that timescale) (I'm not advocating for reproducing MOM5 or MOM4 results, go check out that code if you need that).

I'm fine with a FATAL being thrown. It would be much more informative than the code simply rolling on as if nothing had happened.
Either way is a pain, change the code or change an initial condition. In the long run, changing the initial condition is probably the least painful.

[wfcooke]

Also, can I advocate for an addition of a test case with a restart in the CI system? That is how this was missed in the first place.

[ashao]

@wfcooke, apologies for the confusion, I meant an older tag of MOM6.

Also, do you mean that you are asking for a test case that restarts off of a fixed file stored somewhere on gaea? Otherwise (@Adroft please confirm), I'm pretty sure that the pipeline already tests that the model can reproduce across restarts.

[wfcooke]

@ashao : I think the CI does check across restarts, but it only initializes off cold starts (I'll let @adcroft confirm that) so if you're running off a spunup initial condition like I was and try to update your code, then you can get different answers if a user modifies the code to change the restart variable names. Correct me if I'm wrong but I believe the CI process takes place offsite (not Gaea) so pulling a file from Gaea is not an option. A checked in restartfile would be necessary. I'm not asking for a fully coupled global model. Can a Baltic case test warm starts?

[ashao]

There's the Travis CI that happens when you push commits to Github, but before a PR is accepted, @adcroft or @Hallberg-NOAA will trigger a Gitlab pipeline that runs on Gaea and does pretty extensive testing.

I believe also that you are correct that the current restart check initializes from a coldstart. To test warm starts, Baltic_OM4_05 or Baltic_OM4_025 would be good candidates because they're so small. The downside to that is that not all the test cases will have the same restart variables, so warm-start coverage would be limited to OM4-like coverage. I'll let @adcroft comment on the merits of adding it to the Gitlab CI, but we should bring this up again at the next MOM6 dev call.

[Hallberg-NOAA]

This particular issue was addressed by https://github.com/NOAA-GFDL/MOM6/pull/908. However, some of the proposals for expanded testing to add further checks for any repetition of this issue by testing results with restarts from different versions of the code have not been implemented, largely due to the complexity and added expense of doing so.

Given that there has not been a repetition of this issue in the past 2 years, and that there is now an established procedure for detecting the situation where any future changes in restart variable names would compromise the answers in an ongoing run, I am making the executive decision to close this issue, although it could be reopened if there are objections.