Nodal modulation #725
Nodal modulation #725
Conversation
This commit fixes a few (potential) inconsistencies between open boundary tidal forcing and astronomical tidal forcing. 1. There was an inconsistency in the code that the nodal modulation can be calculated in OBC tidal forcing but not in the astronomical tidal forcing. This commit fixes this bug so that nodal modulation is applied to both forcings. 2. In the previous version of MOM_open_boundary.F90, a different set of tidal parameters can be set for open boundary tidal forcing, leading to potential inconsistency with astronomical tidal forcing. This commit obsoletes those for open boundary tidal forcing. 3. Another important bug fix is that the equilibrium phase is added to the SAL term, which was missing in the original code.
c2xu
force-pushed
the
c2xu/nodal_modulation
branch
from
241f7b7 to
405213a
Compare
There was a problem hiding this comment.
This looks correct to me; thanks for improving how the tidal forcing works.
I also ran two short simulations with the Northwest Atlantic regional model. This PR exactly reproduced results from the existing code (after turning off the nodal modulation, which is improved in this PR but will unavoidably change answers).
Because the OBC tide parameters are removed by this PR, existing regional models with OBC tides will get a fatal but informative error. This makes sense, though, because the parameters are being merged and can't be kept separate for backwards compatibility. Mainly, I just want to tag @yichengt900 so he knows that when we merge this into the CEFI fork we will have to update all of the regional configuration MOM_inputs.
Hallberg-NOAA
added
bug
|
Thank you for this contribution, @c2xu, and for your careful review, @andrew-c-ross. Because this PR will require substantial changes to some runtime parameters, I am going to suggest that we hold this PR up until after our next PR from dev/gfdl to main (which I expect to be in a few weeks, after a pending PR from dev/ncar to main has cleared). The PR after that is going to have a number of changes to parameter default values and names (as were agreed to in the consortium-wide MOM6 dev call in July), and I think that it would be less disruptive if we were to add this PR along with those other changes. |
There was a problem hiding this comment.
I took a more detailed look at this PR (sorry for being two months late), and have a couple of minor comments.
Besides, I also wonder if we could use the initial time of the model as the default for the tidal reference date(s). This is echoing Ed's suggestion in May 2023. It can be easily implemented by passing Time_init from initialize_MOM via initialize_dyn_ variances to tidal_forcing_init.
There was a problem hiding this comment.
There is a process for eliminating or renaming run-time parameters in MOM6. Simply changing existing names will very likely break the solutions for someone else. MOM6 is widely operationally and for ongoing research applications, and our most important principle is that no one changes anyone else's answers without first obtaining their consent.
We therefore have a deliberate process of (1) obtaining community buy-in for all the public interface changes, (2) writing the code so that is will work with both the old and new names that is then available on the MOM6 main branch for about 6 months, e.g. by using one variable to set the default for the other, (3) deprecating the old name by stopping to log it (perhaps with a warning if it is being used), and finally (4) obsoleting the old name after once again obtaining community consensus buy-in. The whole process takes about 2 years.
Usually we only discuss external parameter changes once a year to reduce the amount of foment in the public interfaces that MOM6 users experience, but I have some other changes that I wanted to discuss so I can put this on the agenda for today's MOM6 dev call.
In the mean-time, the biggest step that we could take with this PR would be to use the parameters for the astronomical tides to set the default values for the 4 OBC tide parameters - e.g., read in TIDE_USE_EQ_PHASE and use it as the default value for OBC_TIDE_ADD_EQ_PHASE. Please modify this PR to revise how these runtime parameters (OBC_TIDE_ADD_EQ_PHASE, OBC_TID_ADD_NODAL, OBC_TIDE_REF_DATE and OBC_TIDE_NODAL_REF_DATE) are set, and please remove the lines obsoleting them from MOM_obsolete_params.F90. Assuming that there is a community consensus around using the astronomical values to set the defaults for the OBC values (which I expect there to be), I would be able to approve this PR with such changes.
Added a few obsolete parameters back for compatibility.
I have added those obsolete parameters back. |
| @@ -1194,6 +1194,27 @@ subroutine initialize_obc_tides(OBC, US, param_file) | |||
| "Fixed reference date to use for nodal modulation of boundary tides.", & | |||
| default=0, do_not_log=tides) | |||
|
|
|||
| ! The following parameters are to be obsolete in the future | |||
There was a problem hiding this comment.
These 4 parameters would need to have default arguments if they are to be properly logged in the correct MOM_parameter_doc files.
It may be that we need to add support for a default array of integers to get_param_integer_array(), analogously to what we recently added for get_param_real_array().
If we were to get community buy-in on the idea of moving to use the non-OBC parameters for these tidal parameters, we might deal with this by storing the values set above, adding do_not_log=.true. to these get_param() calls, and then directly adding a warning if variables like OBC_TIDE_ADD_EQ_PHASE are set differently from TIDE_USE_EQ_PHASE. I will try to raise this at this evening's MOM6 dev call, and then provide further guidance depending on how that conversation goes.
There was a problem hiding this comment.
The default arguments for these parameters are already set by TIDE_USE_EQ_PHASE, TIDE_ADD_NODAL, TIDE_REF_DATE and TIDE_NODAL_REF_DATE, so they will always be properly logged. I've tested this.
There was a problem hiding this comment.
It would be more complicated if we want to add a warning if the old variables (e.g., OBC_TIDE_ADD_EQ_PHASE) are set differently from the new ones (e.g., TIDE_USE_EQ_PHASE), but I don't think this should happen if both astronomical and OBC tidal forcings are turned on (and this is the reason for obsoleting the old variables). I did not add a do_not_log option because these parameters were used by OBC tidal forcing only, and I think it would be easier to just remind everyone who uses OBC tidal forcing that these parameters will be obsolete.
|
The proper handling of the changes in the input parameters used in this file should be greatly simplified by #791. I will make specific suggestions for how to modify this PR to handle the parameter changes as soon as #791 is merged into dev/gfdl and this PR is rebased on top of that updated version of dev/gfdl. |
Added the new optional argument old_name to the 8 get_param() routines. This new capability allows for an archaic parameter name to be specified and for appropriate warnings encouraging the user to migrate to using the new name while still setting the parameter as intended, or error messages in the case of inconsistent setting via the archaic name and the correct name. The logging inside of the MOM_parameter_doc files only uses the correct parameter name. Also added the new optional argument set to the 8 read_param routines, to indicate whether a parameter has been found and successfully set. The new set argument is now being used in read_param() calls in obsolete_int(), obsolete_real(), obsolete_char() and obsolete_logical(). Obsolete_logical() in particular was substantially simplified by the use of this new argument, and is now only about half as long as it was. The read_param() set argument is also used in all of the get_param() routines when they are given an old_name argument. The new old_name argument to get_param() is not yet being used in the version of the MOM6 code that is being checked in, but it has been tested extensively by adding or modifying get_param calls in a variant of the initialization code, and it will be used in an updated version of github.com/NOAA-GFDL/pull/725 to gracefully handle the deprecation of 4 parameter names. All answers are bitwise identical, but there are new optional arguments to two widely used interfaces.
|
The handling of the archaic variable names will become very easy once #796 is merged into dev/gfdl, which introduces a new The one other change that I will be suggesting is that parentheses be added to specify the order of the 3 factor sums in the 12 new expressions in |
This commit fixes a few (potential) inconsistencies between open boundary tidal forcing and astronomical tidal forcing.
There was an inconsistency in the code that the nodal modulation can be calculated in OBC tidal forcing but not in the astronomical tidal forcing. This commit fixes this bug so that nodal modulation is applied to both forcings.
In the previous version of MOM_open_boundary.F90, a different set of tidal parameters can be set for open boundary tidal forcing, leading to potential inconsistency with astronomical tidal forcing. This commit obsoletes those for open boundary tidal forcing.
Another important bug fix is that the equilibrium phase is added to the SAL term, which was missing in the original code.