Dimesionally incorrect expressions in apply_oda_tracer_increment()?

[Hallberg-NOAA]

The lines in apply_oda_tracer_increment() where the temperature and salinity increments are applied to the temperature and salinity appear to me to be dimensional inconsistent. The increments (apparently in [C ~> degC] or [S ~> ppt]) are being multiplied by a timestep (in [s]) before being added to the temperatures and salinities themselves (in [C ~> degC] or [S ~> ppt]). This is dimensionally inconsistent. This code is at about lines 717 and 718 of src/ocean_data_assim/MOM_oda_driver.F90, which were added on March 18, 2021 as a part of MOM6 PR#1453.

I believe that the correction might simply be to eliminate the multiplication by the timesteps on these lines, but we might need to figure out whether these lines are being used for any important projects, and hence whether we would need to add a run time bug fix flag,

@MJHarrison-GFDL, could you please take a look at this to see whether I am correct that these lines are dimensionally inconsistent, and also assess which projects might be using them.

[MJHarrison-GFDL]

@Hallberg-NOAA . The code appears to be dimensionally consistent, since the local T_inc and S_inc variables are in units of tracer concentration per time ( [degC s-1] or [ppt s-1] respectively. Subroutine "get_posterior_tracer" converts CS%tv increments (returned from the filter) to tendencies .

[Hallberg-NOAA]

Thanks for this clarification, @MJHarrison-GFDL . With this guidance, I agree that the code itself is dimensionally correct, but it still has a number of problems with the documentation of this subtlety that makes it appear that the MOM_oda_driver code is dimensionally inconsistent. I have a series of suggestions below that would help to clarify this confusion to anyone reading this code:

1. The description of the units of the variables T and S in apply_oda_tracer_increments() are wrong and need to be corrected. Even the names are misleading, since they are the same names as are used elsewhere only for temperatures and salinities, and not their tendencies.
2. The description of the variable units in get_bias_correction_tracer() are wrong, and should be corrected. The dimensional rescaling factors used in this routine are misleading, but consistent with the mixed rescaling presently in the code; they should be US%degC_to_C*US%T_to_s instead of US%degC_to_C, if CS%tv_bc%T were properly rescaled into units of [C T-1 ~> degC s-1], rather than the current mixed units of [C s-1 ~> degC s-1].
3. The use of a thermo_var_ptrs structure to hold temperature and salinity tendencies means that they inherit incorrect documentation of the units of their internal elements. I think it would be much better to simply replace CS%tv%T with CS%T_tend throughout the code to make it clearer what is the nature of variables that are being used.
4. Looking at how they are actually used, it is not clear why we need to use the two thermo_var_ptrs types CS%tv and CS%tv_bc in the ODA_CS. The only fields there that are used are for temperature and salinity (increments), and none of the other machinery associated with thermo_var_ptrs (like structures associated with calling the equation of state) are used, nor could they be used given the atypical units of CS%tv%T and CS%tv%S. The use of these structures is simply confusing and serves (as far as I can tell) no functional purpose, and I think that we should avoid doing so.
5. The variable CS%assim_frequency is not actually a frequency, but a period in an integer number of hours. I think that it would be much better to (1) rename this to CS%assim_interval, (2) use a real number of seconds instead of an integer number of hours, and (3) rescale it to units of [T ~> s].

Is there any reason not to do all this?