Separate self-attraction and loading as a new module#382
Conversation
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #382 +/- ##
==========================================
Coverage 38.01% 38.02%
==========================================
Files 269 270 +1
Lines 77581 77737 +156
Branches 14324 14369 +45
==========================================
+ Hits 29494 29556 +62
- Misses 42736 42815 +79
- Partials 5351 5366 +15
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hallberg-NOAA
left a comment
Hallberg-NOAA
left a comment
There was a problem hiding this comment.
I have read through all of these changes, and they all make sense to me. I am tempted to approve it right now as-is, but for one minor procedural issue.
This PR is bending the usual rules about how we handle obsoleting (or revising) parameter names only after consortium-wide consultation. At this point I suspect there are not yet too many other groups in the MOM6 consortium using the tidal specification parameters for on-line SAL, so we are probably OK with this, but I am going to suggest that we defer acting on this PR until after it has been discussed. @herrwang0, could I ask you to please add the proposed parameter name and interface changes to the shared google drive document that we will be using to discuss parameter name and default changes (ask me directly if you need help finding it), and we will discuss it next Monday (July 17, 2023). I strongly suspect that we are going to find widespread alignment on these changes, but I don't want anyone to be surprised by them.
|
@Hallberg-NOAA This is a very good point. I agree the annual parameter review is the perfect opportunity to officially change these parameter names (and defaults). It would probably also simplify the code by a lot. I will add these proposed changes in the shared document. |
Hallberg-NOAA
left a comment
Hallberg-NOAA
left a comment
There was a problem hiding this comment.
This PR is very close to being ready to go, but please revise it as suggested during the discussion of these changes during the consortium-wide MOM6 dev call on July 17th. Specifically, we should defer the answer-changing default setting of the newly added ANSWER_DATE parameter, and please reexamine the new parameter names to see whether there is a more self-consistent naming scheme that might make sense.
herrwang0
force-pushed
the
add_sal_module_rebase
branch
2 times, most recently
from
be6fe16 to
6b10afc
Compare
|
The parameter name changes are addressed in the new commit. |
Hallberg-NOAA
left a comment
Hallberg-NOAA
left a comment
There was a problem hiding this comment.
This is getting very close, but I have added a few more specific comments about consistency between the new the get_param calls that will need to be addressed before this can be merged in.
Hallberg-NOAA
left a comment
Hallberg-NOAA
left a comment
There was a problem hiding this comment.
Thank you for these final minor modifications. I think that this PR is now ready to go (assuming, of course, that it will pass the regression testing).
Hallberg-NOAA
force-pushed
the
add_sal_module_rebase
branch
from
a2a4ea6 to
1268d99
Compare
There was a problem hiding this comment.
Unfortunately, this PR is changing answers for some cases with the tides enabled, such as nonBous_global and tides_025. Almost certainly the issue is that we have changed order of arithmetic, but I suspect that we will need to track this down and understand it thoroughly before we can merge this PR in to MOM6.
That's odd. There is a flag and a legacy function just to retain old Boussinesq answers, maybe some scenarios that I overlooked and were not covered by my own tests? I just did a quick twin test (latest dev/gfdl vs this PR) with global and the answers were not changed. On the other hand, the flag does not cover for non Boussinesq experiments, the answers are supposed to change. |
|
I have gone back and checked and found that I was in error with my previous comment. (I am sorry about the confusion my previous misstatement created, @herrwang0 !) The global test case in MOM6_examples does reproduce with this PR. It is only nonBous_global and tides_025 where the answers changed, and as noted above, these are Non-Boussinesq test cases. However, when I run the tides_025 case with the added line |
|
If you don't mind, @herrwang0, I think that I am going to try adding the TIDES_ANSWER_DATE code to the semi-Boussinesq cases as well so that we reproduce the answers for these other two test cases. I will then send those (hopefully minor) changes back as PR to your branch. |
|
@herrwang0, I have added a PR onto your feature branch for this PR at github.com/herrwang0/pull/1 which is allowing the non-Boussinesq test cases in the MOM6-examples suite to also reproduce the existing answers with an appropriate value of TIDES_ANSWER_DATE. If you agree with these changes, please merge them into your branch, and then we should be able to merge this PR into dev/gfdl. |
|
@Hallberg-NOAA Thanks for making the patch for preserving the non Boussinesq answers. While we are at this, shall we follow suits and clean up the Boussinesq counterpart? (I coded it in a very redundant way, with the expectation that we could simply remove the else branch some time in the future. But now I think it is probably better with your structure). I can do a quick test and submit a patch later tonight. |
Calculation for self-attraction and loading (SAL) is separated as a new module (MOM_self_attr_load) from module MOM_tidal_forcing, as SAL is a process not limited to tides. The new module includes both online spherical harmonics method and scalar approximation. Read-in method (TIDAL_SAL_FROM_FILE) is kept in the tidal forcing module as it is specific to tides. For the iterative method (USE_PREV_TIDES), the updating part that is tied to the scalar approximation is moved to the new SAL model, while the read-in part remains in the tidal_forcing module. The tidal forcing module now only contains calculations independent from the ocean's state and the only input variables is the current time. * A new parameter CALCULATE_SAL is added, which controls SAL calculation in PressureForce independent of whether tides is on or not. The default of CALCULATE_SAL is TIDES to avoid making changes in old MOM_input. * For the unplit mode, runtime parameters calculate_SAL and use_tides are moved from init subroutines to control structures. This allows safe deallocations of the corresponding module CSs. * A new control structure for the SAL module is used by the dynamical cores and pressure force modules. * For SAL related parameters, their names still incorrectly contain TIDE or TIDAL. This will be addressed in the following commits. * A new diagnostic is added in PressureForce to output calculated SAL fields. Note that the 'e_tidal' diagnostic is unchanged and still includes SAL field for backward compatibility. * Subroutine tidal_forcing_sensitivity, which is used by the barotropic solver to take into account the scalar approximation, is renamed to scalar_SAL_sensitivity. * Documentations are updated for the cited papers.
The output field eta_tidal from subroutine calc_tidal_forcing originally
contains both equilibrium tides and tidal SAL that is based on previous
solutions (TIDAL_SAL_FROM_FILE and USE_PREV_TIDES). This commit
decompose the two fields for better diagnostics.
This also makes answers using USE_PREV_TIDES is not changed if only one
tidal constituent is used.
New diagnostics ('e_tidal_eq' and 'e_tidal_sal') are added in pressure
force modules for the decomposed fields. Note that 'e_tidal' still
includes all tidal fields + SAL fields to be backward compatible.
SSH for SAL is modified for grid points with topography above the reference height z_ref (assumed to be land that can be flooded). Instead of eta anomaly referenced to z_ref, eta anomaly referenced to the bottom depth is used for these grid points.
Remove "TIDAL" and "TIDE" from the relevant input names of the SAL module, as SAL is not specific to the tides. This affects both scalar approximation and the fully online spherical harmonic options. * For scalar SAL, old parameter names are still acceptable, but a WARNING is given if these old names appear in MOM_input. * For read-in SAL, no change is made. * For iterative method (use_prev_tides), the use of TIDE_SAL_SCALAR_VALUE is completely deprecated, as this is a feature that is rarely used. * For harmonic SAL, a relatively recent feature, a hard obsolete is applied, i.e. if the old parameters are specified, a FATAL error is given, unless the new parameters also exist and match the values of the old parameters. List of input names changed: * TIDE_USE_SAL_SCALAR -> USE_SAL_SCALAR * TIDE_SAL_SCALAR_VALUE -> SAL_SCALAR_VALUE * TIDAL_SAL_SHT -> USE_SAL_HARMONICS * TIDAL_SAL_SHT_DEGREE -> SAL_HARMONICS_DEGREE
This commit addresses the issue of bit level answer change with tides in Boussinesq mode. * A runtime parameter TIDES_ANSWER_DATE is added to restore old answers before the SAL module is added. The answer change is due to an reorder of summations of the SAL and tidal forcing terms. * The new version flag only applies to the analytical pressure force in Boussinesq mode, which is virtually the only configuration widely used and included in the test suite. * For Montgomery pressure and non-Boussinesq mode, the code is refactored in a more readable way.
Consistent with the non-Boussinesq option with the new answer option
This commit revises changes of some parameter names from a previous commit. * Logical switch TIDE_USE_SAL_SCALAR is obsolete and replaced by SAL_SCALAR_APPROX. * TIDE_SAL_SCALAR_VALUE is replaced by SAL_SCALAR_VALUE. The old parameter name is still accepted but a warning is given. * Logical switch TIDAL_SAL_SHT is obsolete and replaced by SAL_HARMONICS. * TIDAL_SAL_SHT_DEGREE is obsolete and replaced by SAL_HARMONICS_DEGREE. * RHO_E is replaced by RHO_SOLID_EARTH.
* An incident of incorrect indent is fixed in SAL_int. * Parameters read in SAL_int is moved out from if statements. DO_NOT_LOG is used to prevent logging unused parameters. * Reading SAL_SCALAR_VALUE in tidal_forcing_init is now consistent with SAL_init. * Some unused variables in tidal_forcing_init are removed.
Use TIDES_ANSWER_DATE with an additional if-block to recover the dev/gfdl answers in semi-Boussinesq mode runs with tides enabled, similarly to what is already being done in Boussinesq mode. This changes some answers for the nonBous_global and tides_025 test cases back to those that would be obtained with the code on dev/gfdl. TIDES_ANSWER_DATE is now logged in all cases with tides, so the MOM_parameter_doc files change in some non-Boussinesq cases.
Hallberg-NOAA
force-pushed
the
add_sal_module_rebase
branch
from
8c7ebf4 to
121823d
Compare
|
I am running the updated code on this PR through the pipeline testing to verify that it is all passing. I agree that some refactoring of the Boussinesq code to reduce duplication could be helpful, and if it is available soon, I can redo these tests and accept that update. However, given all of the other outstanding PRs that we are juggling at the moment, I am inclined to take this PR as it stands (assuming that the tests have passed), and then we can take a second separate PR with the refactoring once it is ready. |
|
Some of the commits seem redundant, but since they are all well-commented I will rebase them all into the repo. |
Merge latest DEV-EMC changes
3 participants
Calculation for self-attraction and loading (SAL) is taken out from
MOM_tidal_forcingand housed in a new module (MOM_self_attr_load) as SAL is a process not limited to tides.The new module includes both online spherical harmonics method and scalar approximation. Read-in method (TIDAL_SAL_FROM_FILE) is kept in the tidal forcing module as it is specific to tides. For the iterative method (USE_PREV_TIDES), the updating part that is tied to the scalar approximation is moved to the new SAL model, while the read-in part remains in the tidal_forcing module.
Module
MOM_tidal_forcingis designated only to calculations of the astronomical forcing and periodic tidal SAL that is based on previous solutions from input files. This makes the tidal forcing module now only contain calculations independent from the ocean's state and the only input variable it needs is the current time.CALCULATE_SALis added to control if SAL is calculated. At the moment, the default is linked to ifTIDESis used.Answer changes
Due to the rearrangement of the summations, answers are changed at bit level.
TIDES_ANSWER_DATEruntime flag is added to restore old answers. Any integer > 20230701 gives the new answer. The current default is 20230630.