*Corrected CFC diagnostics and non-Bous CFC fluxes #554
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
Removed the duplicative multiplication by the Boussinesq density and related unit conversion factors in the calculation of the diagnosed mole concentration of the "cfc11" and "cfc12" diagnostics. This unit rescaling is duplicative of the conversion factors in the register_diag_field calls for these diagnostics (at about line 263). This will change these two diagnostics by a factor of RHO_0. This bug arose from separate and independent changes coming from GFDL and NCAR to add the same missing rescaling factor, and it might have gone undetected because of the vast range of valid values for CFC concentrations or because all of our regression testing uses calendar dates that precede the invention and first release of CFCs in the 1930s. Although this commit will correct the code that is on the dev/gfdl branch, care will have to be taken when merging in modified versions of this file from other branches that might have also corrected this bug to avoid reintroducing it (perhaps in reverse). Also replaced the expression for the CFC flux rescaling factor for use with KPP_NonLocalTransport with GV%RZ_to_H, which is equivalent to the previous expression in Boussinesq mode but avoids a multiplication and division by the Boussinesq reference density in non-Boussinesq mode. All Boussinesq answers are identical, but some non-Boussinesq CFCs could be altered in the last bits. This PR will change diagnostics of CFC concentrations by about 3 orders of magnitude. The internal representation of the CFCs is bitwise identical in all Boussinesq cases, but they may change at roundoff in non-Boussinesq cases that use KPP.
Hallberg-NOAA
added
bug
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #554 +/- ##
============================================
- Coverage 37.21% 37.20% -0.01%
============================================
Files 271 271
Lines 80473 80472 -1
Branches 15008 15008
============================================
- Hits 29944 29943 -1
+ Misses 44958 44957 -1
- Partials 5571 5572 +1 ☔ View full report in Codecov by Sentry. |
|
@klindsay28 Is this OK to merge? (I think it's all correct, but it might impact NCAR.) |
marshallward
approved these changes
Feb 29, 2024
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/22489 ✔️ |
This was referenced May 16, 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.
Removed the duplicative multiplication by the Boussinesq density and related unit conversion factors in the calculation of the diagnosed mole concentration of the "cfc11" and "cfc12" diagnostics. This unit rescaling is duplicative of the conversion factors in the
register_diag_field()calls for these diagnostics (at about line 263). This will change these two diagnostics by a factor of RHO_0. This bug arose from separate and independent changes coming from GFDL and NCAR to add the same missing rescaling factor, and it might have gone undetected because of the vast range of valid values for CFC concentrations or because all of our regression testing uses calendar dates that precede the invention and first release of CFCs in the 1930s. Although this commit will correct the code that is on the dev/gfdl branch, care will have to be taken when merging in modified versions of this file from other branches that might have also corrected this bug to avoid reintroducing it (perhaps in reverse).Also replaced the expression for the CFC flux rescaling factor for use with
KPP_NonLocalTransport()withGV%RZ_to_H, which is equivalent to the previous expression in Boussinesq mode but avoids a multiplication and division by the Boussinesq reference density in non-Boussinesq mode. All Boussinesq answers are identical, but some non-Boussinesq CFCs could be altered in the last bits.This PR will change diagnostics of CFC concentrations by about 3 orders of magnitude. The internal representation of the CFCs is bitwise identical in all Boussinesq cases, but they may change at roundoff in non-Boussinesq cases that use KPP.