-
Notifications
You must be signed in to change notification settings - Fork 65
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
Do not allocate ustar and tau_mag together #464
Do not allocate ustar and tau_mag together #464
Conversation
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #464 +/- ##
============================================
- Coverage 37.90% 37.88% -0.02%
============================================
Files 270 270
Lines 78100 78125 +25
Branches 14436 14456 +20
============================================
Hits 29600 29600
- Misses 43123 43146 +23
- Partials 5377 5379 +2
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
There are a surprising number of tau_mag
/ustar
updates with unnecessary if (associate()
blocks inside of them. Although the local value of tau_mag
is used to compute ustar
, it does potentially impact the performance on platforms with poor branch prediction.
There are also instances of such loops which have the stress magnitude nested inside of the expression for ustar
. At the very least, there is some inconsistency here.
Although I am unsure about the frequency of this function call and the overall impact on performance, I would recommend rewriting these loops to avoid such if-blocks within the loops.
8a66adc
to
9de6ce7
Compare
Modified MOM_surface_forcing_gfdl.F90 and MOM_surface_forcing.F90 to allocate either ustar or tau_mag, but not both, in the forcing and mech_forcing types, depending on whether the model is in Boussinesq mode. Also added tests to convert_IOB_to_forces in the FMS_cap code and to routines in MOM_surface_forcing in the solo_driver code to ensure that only arrays that are associated are set. All answers are bitwise identical, but checksum statements for the unused arrays are eliminated when DEBUG = True.
e89738c
to
2137956
Compare
I have revised this PR to move all of the |
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 all looks more consistent now.
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/20777 ✔️ |
Modified MOM_surface_forcing_gfdl.F90 and MOM_surface_forcing.F90 to allocate either ustar or tau_mag, but not both, in the forcing and mech_forcing types, depending on whether the model is in Boussinesq mode. Also added tests to convert_IOB_to_forces in the FMS_cap code and to routines in MOM_surface_forcing in the solo_driver code to ensure that only arrays that are associated are set. All answers are bitwise identical, but checksum statements for the unused arrays are eliminated when DEBUG = True.