GFDL->main PR regression and bug fixes #782
Conversation
The MEKE GM computation of `src` and `src_GM`, a diagnostic array, were placed in a single loop. The similar RHS of each expression made it unfavorable to use FMAs on the `src` update. Older production runs depending on this FMA were seeing answer changes. This patch restores the FMA loop update of `src` by separating `src` and `src_GM` into separate loops.
This patch makes several adjustments to MOM_MEKE.F90 and MOM_hor_visc.F90 to ensure that the Laplacian and biharmonic friction coefficients are computed separately, and only if their respective terms are enabled. This resolves some subtle bugs where the default biharmonic value of -1 was applied to the Laplacian case, even when the biharmonic MEKE friction was disabled.
|
The regression is due to the Although this is a regression into NOAA-GFDL:gfdl-to-main-2024-11-27 it should not cause a regression of NOAA-GFDL:gfdl-to-main-2024-11-27 into |
marshallward
force-pushed
the
ebt_src_gm_split
branch
from
5f4bbc2 to
4f1fdd9
Compare
|
Thanks for the PR. I have reviewed the changes and made a few comments. Please let me know if you have any questions.
…________________________________
From: Marshall Ward ***@***.***>
Sent: Thursday, December 19, 2024 12:42 PM
To: NOAA-GFDL/MOM6 ***@***.***>
Cc: Wenda Zhang ***@***.***>; Mention ***@***.***>
Subject: [NOAA-GFDL/MOM6] GFDL->main PR regression and bug fixes (PR #782)
Several fixes to the candidate PR to main:
* Fixed bugs due to introduction of MEKE biharmonic friction
* Restored performance due to new visc-limit flag in MEKE FrictWork. This is now applied separately, and only if EY24_EBT_BR is enabled.
* Restored performance due to multiple damped MEKE diagnostics. Nearly every calculation is now conditional.
* Bugfix to an internal tide control struct declaration
There is also a commit to restore answers related to MEKE backscatter, but this is still under some discussion. I'm leaving it in for now, but we may change or remove this one if NCAR approves.
This needs review from one of the following:
* @Wendazhang33<https://github.com/Wendazhang33>
* @ElizabethYankovsky<https://github.com/ElizabethYankovsky>
Further testing will eventually be needed from @alperaltuntas<https://github.com/alperaltuntas> and @jiandewang<https://github.com/jiandewang> but this can wait until Wenda and/or Elizabeth have reviewed the modificatins.
________________________________
You can view, comment on, or merge this pull request online at:
#782
Commit Summary
* e59e2d6<e59e2d6> MEKE: Split src and src_GM compute loops
* 0a7ca20<0a7ca20> MEKE: Conditionally compute biharmonic FrCoeff
* 423e388<423e388> MEKE: Revert Backscatter expressions
* 7e32c83<7e32c83> MEKE: Move flag test outside of FrictWork loop
* f20a98c<f20a98c> MEKE: Move damping diagnostics outside MEKE loop
* 5f4bbc2<5f4bbc2> Diffusivity: Revert int_tide_CS to pointer
File Changes
(3 files<https://github.com/NOAA-GFDL/MOM6/pull/782/files>)
* M src/parameterizations/lateral/MOM_MEKE.F90<https://github.com/NOAA-GFDL/MOM6/pull/782/files#diff-2bf6c52474ad3142c4902b0f94ed67365c19e0bc7a6a61c4dcbc4e453330d258> (250)
* M src/parameterizations/lateral/MOM_hor_visc.F90<https://github.com/NOAA-GFDL/MOM6/pull/782/files#diff-66ca61ccaca43a9217e857b349d1ffc3fdfcfa65b04995b6e62b0d66d1e1a16d> (218)
* M src/parameterizations/vertical/MOM_set_diffusivity.F90<https://github.com/NOAA-GFDL/MOM6/pull/782/files#diff-f0477334759277205ea346ab76bda77c93199f160413c86e25d59a8aa7088c00> (2)
Patch Links:
* https://github.com/NOAA-GFDL/MOM6/pull/782.patch
* https://github.com/NOAA-GFDL/MOM6/pull/782.diff
—
Reply to this email directly, view it on GitHub<#782>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKBBAQFWVTKXSRBKQCHBM432GMASFAVCNFSM6AAAAABT5P6X6WVHI2DSMVQWIX3LMV43ASLTON2WKOZSG42TCMBVG4YDGOI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
marshallward
force-pushed
the
ebt_src_gm_split
branch
4 times, most recently
from
4f000cd to
47487a4
Compare
The if-test inside of the FrictWork loops are likely to impede performance. Even if the total work is reduced, they are likely to interrupt pipelines. When EY24_EBT_BS is disabled, they will clearly reduce performance. This patch moves those tests outside of the if-block and applies them separately. (Calculation would be slightly improved if the meaning of the flag were reversed, but I don't want to make additional changes.)
The damping MEKE loop also included updates to multiple diagnostics, even if they were not registered. This would presumably have a negative impact on performance. This patch moves each diagnostic into a separate loop. It also conditionally precomputes the damping and damp_rate parameters, which are now stored as 2d arrays rather than in-loop scalars. As before, the MEKE calculation is left unchanged in order to preserve bit reproducibility.
The redefining of int_tide_CS control struct in set_diffusivity_init caused errors in debug-mode for Intel compilers. The issue appears to be an internal function that expects a pointer rather than the type. This patch reverts this back to a pointer. We can revisit this if there is a need to reduce reliance on pointers.
marshallward
force-pushed
the
ebt_src_gm_split
branch
from
47487a4 to
5db4f28
Compare
|
I've made several adjustments based on suggestions by @Wendazhang33
I believe this is now ready for review, and merge into the PR to main. |
This patch updates the expression for FrictWork_bh (biharmonic frictional work) when the FrictWork_bug flag is enabled. The new form is symmetric to rotations when FMA instructions are enabled.
There was a problem hiding this comment.
I have examined all of these changes, and I have checked out the updated code and used it to run the MOM6-examples regression suite. The change in diagnostics in the TC testing is properly explained. I am satisfied that this is correct and that it is likely to address the issues that were identified with the 2024-11-27 GFDL-to-main (mom-ocean#1647) pull request.
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/25932 ✔️ 🟡 This passes our own tests, I will merge this into the PR for consortium review. |
marshallward
merged commit c346c73
into
NOAA-GFDL:gfdl-to-main-2024-11-27
Several fixes to the candidate PR to main:
EY24_EBT_BRis enabled.There is also a commit to restore answers related to MEKE backscatter, but this is still under some discussion. I'm leaving it in for now, but we may change or remove this one if NCAR approves.
This needs review from one of the following:
Further testing will eventually be needed from @alperaltuntas and @jiandewang but this can wait until Wenda and/or Elizabeth have reviewed the modificatins.