Skip to content

Vert friction: Column loops moved in layers#973

Merged
Hallberg-NOAA merged 2 commits into
NOAA-GFDL:dev/gfdlfrom
marshallward:vertvisc_columns
Sep 22, 2025
Merged

Vert friction: Column loops moved in layers#973
Hallberg-NOAA merged 2 commits into
NOAA-GFDL:dev/gfdlfrom
marshallward:vertvisc_columns

Conversation

@marshallward
Copy link
Copy Markdown
Member

This patch moves the k-column loops inside of ji-layer loops, rather than outer-k loops of layers.

The primary motivation is to restore performance at high-bandwidth runs, which were insufficiently tested during development of the k-j-i form.

The inner-column loops show improved performance for both low and high-bandwidth runs.

The high-bandwidth benchmark case: (128-core, 256x128 x 75 layer)

                                   Profile   Reference
      (Ocean vertical viscosity):   7.158s,  15.047s (-52.4%)

The low bandwidth case: (1-core, 32x32 x 75 layer)

                                   Profile   Reference
      (Ocean vertical viscosity):   3.911s,   4.788s (-18.3%)

For the GFDL OM5 production configuration at 503, runtimes of the slowest ranks were reduced in proportion to the high-bandwidth case above.

For the reference dev/gfdl,

                                      hits          tmin          tmax          tavg
(Ocean vertical viscosity)             288      4.303819     21.483670     14.452196

After apply this patch, times reduce ~40%

                                      hits          tmin          tmax          tavg
(Ocean vertical viscosity)             288      0.976130     13.398768      8.689331
  • Moving to columns allowed for removal of many do_i tests, since the test is applied before starting the loop.

  • The touch_ij dummy function was removed, since we're no longer trying to force an IPO optimization.

  • The shelf requires a re-calculation of the various thickness averages (h_arith, etc). These could be saved as 1D if it becomes a problem.

  • Several indexing bugs are now resolved:

    • Many instances of j=G%isc,G%jec. These went unnoticed because isc=jec in domain-local indexing, which is default.

    • One ice shelf loop was incorrectly i=is,je. Thanks to Claire Yung for reporting.

    Changing to inner-column loops nullifies most of these issues.

  • In addition to the usual regression testing, I also found no regressions in selected ice shelf configurations.

@marshallward
Copy link
Copy Markdown
Member Author

@claireyung if you are able to test this out, that would be a big help.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.32%. Comparing base (dcae198) to head (511590f).

Additional details and impacted files
@@             Coverage Diff              @@
##           dev/gfdl     #973      +/-   ##
============================================
- Coverage     37.39%   37.32%   -0.07%     
============================================
  Files           306      306              
  Lines         93749    93511     -238     
  Branches      17977    17976       -1     
============================================
- Hits          35053    34903     -150     
+ Misses        52096    52017      -79     
+ Partials       6600     6591       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claireyung
Copy link
Copy Markdown

Thanks @marshallward. All my icemount/seamount expts running as expected with this commit.

However, I just realised that my ISOMIP 3D domain experiments are not working as well, and this commit (or just replacing the index as you suggested) doesn't seem to fix it. I will reply to the issue #971 with a link to the config. Sorry about this.

@marshallward
Copy link
Copy Markdown
Member Author

I ran commit 8b63869 with the kji vertvisc reordering, and added the i=is,je fix, and it ran without crashing.

I don't know what to compare for bit repro, but it seems to me that this problem may not be related to MOM_vert_friction.F90.

I'll keep looking though, and will move the discussion to #971.

@marshallward marshallward force-pushed the vertvisc_columns branch 6 times, most recently from de53fb2 to b253f58 Compare September 22, 2025 19:39
@marshallward
Copy link
Copy Markdown
Member Author

I modified this PR to include an additional commit which documents and fixes the referenced index problems.

The columnar commit was also modified to reduce whitespace and formatting changes, so as to reduce the number of changed lines.

Copy link
Copy Markdown
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have carefully examined every line of the refactoring changes in the second commit of this PR and the bug-fix changes in the first commit in this PR, and I believe them all to be correct. There would be some scope for a bit more minor cleanup here in a subsequent commit, such avoiding logical tests that have become duplicated after these changes, or reformatting some comments now that the declaration statements are much shorter, but none of these should change answers or have a very large impact on performance. This commit is ready to go in as it stands.

This patch fixes two class of index errors in multiple functions of
`MOM_vert_friction.F90`:

* `j=G%isc,G%jec` had been incorrectly applied to multiple loops.  This
  went undetected because we almost exclusively use local indexing where
  `G%isc == G%jsc`, but is nonetheless a serious error.  Thanks to Jorge
  Luis Gálvez Vallejo for reporting.

* One errant loop in the shelf code had `i=is,je`.  This was undetected
  due to poor ice shelf coverage testing.  Thanks to Claire Yung for
  reporting.
This patch moves the k-column loops inside of ji-layer loops, rather
than outer-k loops of layers.

The primary motivation is to restore performance at high-bandwidth runs,
which were insufficiently tested during development of the k-j-i form.

The inner-column loops show improved performance for both low and
high-bandwidth runs.

The high-bandwidth benchmark case: (128-core, 256x128 x 75 layer)
```
                                   Profile   Reference
      (Ocean vertical viscosity):   7.158s,  15.047s (-52.4%)
```
The low bandwidth case: (1-core, 32x32 x 75 layer)
```
                                   Profile   Reference
      (Ocean vertical viscosity):   3.911s,   4.788s (-18.3%)
```
For the GFDL OM5 production configuration at 503, runtimes of the slowest ranks
were reduced in proportion to the high-bandwidth case above.

For the reference dev/gfdl,
```
                                      hits          tmin          tmax          tavg
(Ocean vertical viscosity)             288      4.303819     21.483670     14.452196
```
After apply this patch, times reduce ~40%
```
                                      hits          tmin          tmax          tavg
(Ocean vertical viscosity)             288      0.976130     13.398768      8.689331
```

* Moving to columns allowed for removal of many `do_i` tests, since the test is
  applied before starting the loop.

* The `touch_ij` dummy function was removed, since we're no longer trying to
  force an IPO optimization.

* The shelf requires a re-calculation of the various thickness averages
  (h_arith, etc).  These could be saved as 1D if it becomes a problem.

* In addition to the usual regression testing, I also found no regressions in
  selected ice shelf configurations.
@Hallberg-NOAA
Copy link
Copy Markdown
Member

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/28922.

@Hallberg-NOAA Hallberg-NOAA merged commit 067ed5f into NOAA-GFDL:dev/gfdl Sep 22, 2025
52 checks passed
@Hallberg-NOAA Hallberg-NOAA added bug Something isn't working refactor Code cleanup with no changes in functionality or results labels Sep 23, 2025
@marshallward marshallward deleted the vertvisc_columns branch November 18, 2025 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working refactor Code cleanup with no changes in functionality or results

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants