Nodal modulation #725
Nodal modulation #725
Conversation
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.
|
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.
I have added those obsolete parameters back. |
|
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 |
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.
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//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.
|
Now that #796 has been merged into dev/gfdl, after this PR is rebased atop dev/gfdl it should be relatively straight-forward to modify this PR to add |
59211b6 to
cb68008
Compare
Thanks @Hallberg-NOAA for the new functionality. I tested the new code and everything seems to work as expected. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #725 +/- ##
===========================================
Coverage ? 21.94%
===========================================
Files ? 136
Lines ? 32819
Branches ? 5835
===========================================
Hits ? 7202
Misses ? 25060
Partials ? 557 ☔ View full report in Codecov by Sentry. |
cb68008 to
9e2c6be
Compare
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.
9e2c6be to
204fcb7
Compare
There was a problem hiding this comment.
This commit now properly manages the changes in parameter names, while adding a valuable new capability with appropriate parentheses to specify the order of arithmetic in all the new expressions that add 3 variables. All of the concerns raised with the previous versions have now been satisfactorily addressed.
|
This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/26106. |
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.
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.