-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
+Add MOM_check_scaling.F90 and MOM_scaling_check.F90 #49
+Add MOM_check_scaling.F90 and MOM_scaling_check.F90 #49
Conversation
Added two new modules, the MOM6-specific MOM_check_scaling.F90 and the generic framework module MOM_scaling_check.F90, to assess the uniqueness of the unit scaling factors for all of the variables used by MOM6. If there are overlaps in scaling factors for different units, this also identifies and suggests alternate scaling factors with less overlaps. This commit includes the introduction of the new publicly visible routines check_scaling_factors(), scales_to_powers() and check_MOM6_scaling_factors. This new capability does not do anything for sufficiently low levels of model verbosity, and it is silent if the scaling factors are unique, or if less than 2 dimensions are being rescaled. All answers and output files are bitwise identical, but there can be additional messages to stdout.
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #49 +/- ##
============================================
+ Coverage 28.94% 29.02% +0.07%
============================================
Files 242 244 +2
Lines 71553 71850 +297
============================================
+ Hits 20714 20854 +140
- Misses 50839 50996 +157
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice innovation, and as usual, properly working code. However:
- I don't understand the use of two modules here. These could easily be one module and not change the functionality currently coded. The naming MOM_check_scaling and MOM_scaling_check do not help me know which level I'm looking at. I note the module descriptions are identical which must be corrected. Some of subroutine descriptions are so similar one would think the modules duplicate each other when infact one does the work and the other does the set up. One module seems to make sense, or if there's a real reason to not join them, a clearer description of the two levels.
- Please change the verbosity in one of the TCs so that this code gets 100% coverage.
- The regular comments about how units work have very useful information (e.g. L13-17 of the new modules) but this will not be found or referenceable. I suggest moving this into a dedicated .dox about units and scaling so we can refer to this documentation.
Thank you for the review @adcroft
|
|
Renamed the framework module MOM_scaling_check.F90 to MOM_unique_scales.F90 to help differentiate it from MOM_check_scaling.F90, and renamed the subroutine check_scaling_factors() as check_scaling_uniqueness(). Also added _Dimensional_consistency.dox to describe the dimensional consistency testing. This commit should address the issues raised in the review of MOM6 PR mom-ocean#49. All answers and output are bitwise identical.
I have renamed one of the two new modules for help differentiate them, and I have added a new .dox file describing the unit scaling. However, I think that changes to the overall structure of the TC testing to exercise this new code goes beyond the scope of this PR. I now believe that this PR is ready to be reevaluated. |
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14676 ✔️ |
Will merge after approval from @adcroft |
Renamed the framework module MOM_scaling_check.F90 to MOM_unique_scales.F90 to help differentiate it from MOM_check_scaling.F90, and renamed the subroutine check_scaling_factors() as check_scaling_uniqueness(). Also added _Dimensional_consistency.dox to describe the dimensional consistency testing. This commit should address the issues raised in the review of MOM6 PR #49. All answers and output are bitwise identical.
Added two new modules, the MOM6-specific MOM_check_scaling.F90 and the generic
framework module MOM_scaling_check.F90, to assess the uniqueness of the unit
scaling factors for all of the variables used by MOM6. If there are overlaps in
scaling factors for different units, this also identifies and suggests alternate
scaling factors with less overlaps. This commit includes the introduction of
the new publicly visible routines check_scaling_factors(), scales_to_powers()
and check_MOM6_scaling_factors. This new capability does not do anything for
sufficiently low levels of model verbosity, and it is silent if the scaling
factors are unique, or if less than 2 dimensions are being rescaled. All
answers and output files are bitwise identical, but there can be additional
messages to stdout.