+(*)Fix numerous issues with MOM_stoch_eos and Stanley parameterizations #271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #271 +/- ##
============================================
- Coverage 37.11% 37.10% -0.01%
============================================
Files 263 263
Lines 73453 73494 +41
Branches 13679 13696 +17
============================================
+ Hits 27264 27272 +8
- Misses 41160 41194 +34
+ Partials 5029 5028 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hallberg-NOAA
added
bug
|
There are unrelated OBC changes showing up in here. Are they in another PR too? |
|
Ah - I see those are part of this PR's description even though the changes are unrelated... |
adcroft
approved these changes
Dec 19, 2022
There was a problem hiding this comment.
Tested twice, one original and one with a patch to convert some warnings to fatal.
https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/17715
https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/17733
mom6bot will need to update MOM6-examples due to doc string changes.
|
Conflicts will need to be fixed if this is to be rebased. |
Hallberg-NOAA
force-pushed
the
fix_stoch_EOS_scaling
branch
from
c2e6f40 to
8143b4b
Compare
Corrected several bugs that would prevent cases with STOCH_EOS = True or that set a non-negative value of STANLEY_COEF from running at all. There was also extensive refactoring of the MOM_stoch_eos.F90 code. Specifically this commit makes the following changes: - The new routine stoch_eos_register_restarts was added to register the restart field associated with the MOM_stoch_EOS module before the restarts are read and before restart_registry_lock is called, which was causing any test case with STOCH_EOS=True or a positive value of STANLEY_COEF to have a fatal error. - Added a missing dimensional rescaling factor in the get_param call for KD_SMOOTH in MOM_stoch_eos_init, which would have caused cases exercising the MOM_stoch_eos options to fail the dimensional consistency tests. - MOM_stoch_eos_init was changed from a subroutine into a function that returns a logical value indicating whether this routine is used further. The order of the arguments was modified to match the order used in every other init call. - The new routine post_stoch_eos_diags was added to write diagnostics associated with the stoch_eos module. - The MOM_stoch_eos_CS type was made opaque. - The unused diag argument was removed from MOM_stoch_eos_run. - Unit arguments were added to the get_param calls for STANLEY_COEFF and STANLEY_A. - Four arrays in the MOM_stoch_eos_CS type that had been declared to optionally use static memory allocation were modified to be simple allocatables, as they are not used in the vast majority of MOM6 cases, and there is no reason to always assign memory to them. - The register_restart_field call for "stoch_eos_pattern" was revised to use the newer, more direct form rather than working via a vardesc type. - Return statements were added to several of the MOM_stoch_eos routines in the cases where they are not supposed to do anything. - The comments describing several real variables in the MOM_stoch_eos module were added or modified to describe their units using the standard format. - The module use statements at the start of the MOM_stoch_eos module were updated to reflect these changes. There were also parallel changes in MOM.F90: - Added use_stochastic_EOS element to the MOM_control_struct to indicate whether the stoch_eos calls are to be used. - MOM_stoch_eos_init is only called if temperature and salinities are state variables, as it makes no sense to call it otherwise. - Calls to stoch_eos routines were updates to reflect the new interfaces. - The contents of the MOM_stoch_eos_CS type are no longer used in MOM.F90. All answers are bitwise identical in cases that do not use dimensional rescaling, but answers will change (be corrected) in some cases that do use dimensional consistency tests. Several public interfaces to MOM_stoch_eos routines were altered, and there are changes to multiple MOM_parameter_doc files due to the new units, and due to the fact that stoch_EOS parameters are no longer being logged in cases where they are meaningless.
Corrected the domain extents used in EOS calls that are triggered with USE_STANLEY_GM and USE_STANLEY_ISO. These bugs were accidentally introduced when the changes adding the MOM_stoch_eos code to the main branch of MOM6 were merged with changes on the dev/gfdl branch. Also added a test for cases when USE_STANLEY_GM is set to true but STANLEY_COEF is negative to reset the internal versions of this flag to false with a sensible warning message rather than encountering segmentation faults. All solutions are bitwise identical in cases that worked before.
Corrected the diagnostics rho_pgf, rho_stanley_pgf and p_stanley to only be calculated if they would be written, give identical output when dimensional rescaling is applied, and be documented with the right units. Also added a test for cases when USE_STANLEY_GM is set to true but STANLEY_COEF is negative to reset the internal versions of this flag to false with a sensible warning message rather than encountering segmentation faults. All solutions are bitwise identical in cases that worked before, but there are changes in some diagnostics when they are dimensionally rescaled.
Also added tests for cases when USE_STANLEY_ISO or USE_STANLEY_ML are set to true but STANLEY_COEF is negative to reset the internal versions of these flags to false with a sensible warning message rather than encountering segmentation faults. All solutions are bitwise identical in cases that worked before.
Added a missing dimensional rescaling factor in the get_param call for the recently added variable DT_OBC_SEG_UPDATE_OBGC in initialize_MOM, which would have caused certain cases to fail the dimensional consistency tests. All answers are bitwise identical in cases that do not use dimensional rescaling, but answers will change (and be corrected) in some cases that use dimensional consistency tests.
Use dimensionally rescaled units when preparing fields to write to the MOM_IC, and then use the conversion argument to the register_restart_field call to undue this scaling, following the pattern for other calls. All answers and output are bitwise identical.
Hallberg-NOAA
force-pushed
the
fix_stoch_EOS_scaling
branch
from
8143b4b to
b8c3845
Compare
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/17750 ✔️ |
This was referenced Apr 6, 2023
marshallward
pushed a commit
that referenced
this pull request
Apr 9, 2024
Merge latest main (Feb 28, 2024)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR corrects several bugs that would prevent cases with STOCH_EOS = True or that set a non-negative value of STANLEY_COEF from running at all, and then when it runs it corrects other bugs that cause it to give incorrect answers, as evident in its inability to reproduce across processors, and to fail to pass dimensional consistency testing. There was also extensive refactoring of the MOM_stoch_eos.F90 code. Specifically this PR makes the following changes:
The new routine stoch_eos_register_restarts was added to register the restart
field associated with the MOM_stoch_EOS module before the restarts are read
and before restart_registry_lock is called, which was causing any test case
with STOCH_EOS=True or a positive value of STANLEY_COEF to have a fatal error.
Added a missing dimensional rescaling factor in the get_param call for
KD_SMOOTH in MOM_stoch_eos_init, which would have caused cases exercising the
MOM_stoch_eos options to fail the dimensional consistency tests.
MOM_stoch_eos_init was changed from a subroutine into a function that returns
a logical value indicating whether this routine is used further. The order of
the arguments was modified to match the order used in every other init call.
The new routine post_stoch_eos_diags was added to write diagnostics associated
with the stoch_eos module.
The MOM_stoch_eos_CS type was made opaque.
The unused diag argument was removed from MOM_stoch_eos_run.
Unit arguments were added to the get_param calls for STANLEY_COEFF and
STANLEY_A.
Four arrays in the MOM_stoch_eos_CS type that had been declared to optionally
use static memory allocation were modified to be simple allocatables, as they
are not used in the vast majority of MOM6 cases, and there is no reason to
always assign memory to them.
The register_restart_field call for "stoch_eos_pattern" was revised to use the
newer, more direct form rather than working via a vardesc type.
Return statements were added to several of the MOM_stoch_eos routines in the
cases where they are not supposed to do anything.
The comments describing several real variables in the MOM_stoch_eos module
were added or modified to describe their units using the standard format.
The module use statements at the start of the MOM_stoch_eos module were
updated to reflect these changes.
Added use_stochastic_EOS element to the MOM_control_struct to indicate whether
the stoch_eos calls are to be used.
MOM_stoch_eos_init is only called if temperature and salinities are state
variables, as it makes no sense to call it otherwise.
Calls to stoch_eos routines were updates to reflect the new interfaces.
The contents of the MOM_stoch_eos_CS type are no longer used in MOM.F90.
Corrected the domain extents used in EOS calls that are triggered with
USE_STANLEY_GM and USE_STANLEY_ISO. These bugs were accidentally introduced
when the changes adding the MOM_stoch_eos code to the main branch of MOM6 were
merged with changes on the dev/gfdl branch.
Added tests for cases when USE_STANLEY_GM, USE_STANLEY_PGF,USE_STANLEY_ISO or
USE_STANLEY_ML are set to true but STANLEY_COEF is negative to reset the
internal versions of this flag to false with a sensible warning message rather
than encountering segmentation faults.
Corrected the diagnostics rho_pgf, rho_stanley_pgf and p_stanley to only be
calculated if they would be written, give identical output when dimensional
rescaling is applied, and be documented with the right units.
Added a missing dimensional rescaling factor in the get_param call for the
recently added variable DT_OBC_SEG_UPDATE_OBGC in initialize_MOM, which would
have caused certain cases to fail the dimensional consistency tests.
Use dimensionally rescaled units when preparing the interface height fields to
write to the MOM_IC file, and then use the conversion argument to the
register_restart_field call to undue this scaling.
Several public interfaces to MOM_stoch_eos routines were altered, and there are changes to multiple MOM_parameter_doc files due to the new units, and due to the fact that stoch_EOS parameters are no longer being logged in cases where they are meaningless. All answers are bitwise identical in cases that do not use dimensional rescaling or the USE_STANLEY parameters, but answers will change (i.e. are corrected) in some cases that do use dimensional consistency tests or the STOCH_EOS or USE_STANLEY parameterizations.
This commits in this PR include: