Skip to content

vectorize helfsurface() and related subroutines#348

Merged
sdrabenh merged 8 commits intomainfrom
feature/wjiang/cleanup_helsurface
Apr 15, 2025
Merged

vectorize helfsurface() and related subroutines#348
sdrabenh merged 8 commits intomainfrom
feature/wjiang/cleanup_helsurface

Conversation

@weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented May 28, 2024

Improves performance of helfsurface() and related subroutines by vectorization and elimination of some unnecessary calculations.

For one day M09 simulation, the helfsurface subroutine takes about 11.5 seconds. After the change, it takes about 8.5 second. The performance is about 25% improvement. For comparison, the Louis subroutine takes about 0.9 second

Revised code does not work with older compilers (ifort 2021.6 and before) because they cannot handle the "where" construct properly, especially for the nested where .

Requires GEOS-ESM/GEOSldas#796 (merged 2 April 2025)

@github-actions
Copy link

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found:

@github-actions
Copy link

github-actions bot commented Jun 1, 2024

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found:

@github-actions
Copy link

github-actions bot commented Jun 1, 2024

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found:

@weiyuan-jiang weiyuan-jiang added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Jun 1, 2024
@weiyuan-jiang
Copy link
Contributor Author

For one day M09 simulation, the helfsurface subroutine takes about 11.5 seconds. After the change, it takes about 8.5 second. The performance is about 25% improvement. For comparison, the Louis subroutine takes about 0.9 second

@weiyuan-jiang weiyuan-jiang force-pushed the feature/wjiang/cleanup_helsurface branch from e95f7a3 to 7f4e86f Compare June 1, 2024 12:11
Co-authored-by: Matt Thompson <matthew.thompson@nasa.gov>
@mathomp4
Copy link
Member

mathomp4 commented Jun 3, 2024

@weiyuan-jiang Do you know if the GCM uses Louis or Helf? I ask only because this might be non-zero-diff for Helf and so if the GCM uses Helf, it should be marked as non-zero-diff.

@weiyuan-jiang
Copy link
Contributor Author

weiyuan-jiang commented Jun 3, 2024

@weiyuan-jiang Do you know if the GCM uses Louis or Helf? I ask only because this might be non-zero-diff for Helf and so if the GCM uses Helf, it should be marked as non-zero-diff.

GCM is using helfsurface by default

@weiyuan-jiang
Copy link
Contributor Author

weiyuan-jiang commented Jun 3, 2024

I saw it crashes. Does CI use debug built type ? @mathomp4 . The ifort 2021.6 (and before it) could not handle "where" construct properly, especially for the nested where .

@mathomp4
Copy link
Member

mathomp4 commented Jun 3, 2024

Yes. By default our CI runs with debug build type. Both Intel and GNU

I'll need to see if I can update the images for a newer Intel. That might take a bit of work

@mathomp4
Copy link
Member

Actually, this is now usable in GEOSgcm v11.7 as we have moved to Intel Fortran Classic 2021.13

@mathomp4 mathomp4 marked this pull request as ready for review February 26, 2025 18:37
@mathomp4 mathomp4 requested review from a team as code owners February 26, 2025 18:37
@biljanaorescanin
Copy link
Contributor

@mathomp4 Let me retest it before we merge since I don't remember did I run all the tests.

@mathomp4
Copy link
Member

@mathomp4 Let me retest it before we merge since I don't remember did I run all the tests.

@biljanaorescanin Oooh. Are you testing with LDAS? If so, you might not have the needed updates for g5_modules. I can prepare a PR for you if you like so that it has the updates?

@biljanaorescanin
Copy link
Contributor

@mathomp4 Let me retest it before we merge since I don't remember did I run all the tests.

@biljanaorescanin Oooh. Are you testing with LDAS? If so, you might not have the needed updates for g5_modules. I can prepare a PR for you if you like so that it has the updates?

That would be great if you could do that @mathomp4

@mathomp4
Copy link
Member

@mathomp4 Let me retest it before we merge since I don't remember did I run all the tests.

@biljanaorescanin Oooh. Are you testing with LDAS? If so, you might not have the needed updates for g5_modules. I can prepare a PR for you if you like so that it has the updates?

That would be great if you could do that @mathomp4

@biljanaorescanin here you go:

GEOS-ESM/GEOSldas#796

I named the branch there the same as this PR. Note: I'm 99% certain this will be NZD. So, it might need to be tested with and without the GMAO_Shared branch?

@biljanaorescanin
Copy link
Contributor

GCM testing summary (if we test with with GEOSgcm_GridComp Develop branch and this branch) :

  1. 1day AMIP is zero diff
  2. 1day REPLAY is zero diff
  3. 1d noreplay is zero diff.

Why we need develop GEOSgcm_GridComp for this PR is because there is a bug and I believe @mathomp4 is working on some resolution. If we don't use develop CMake will fail due to No SOURCES given to target: w3adc. issue is here

@mathomp4
Copy link
Member

GCM testing summary (if we test with with GEOSgcm_GridComp Develop branch and this branch) :

  1. 1day AMIP is zero diff
  2. 1day REPLAY is zero diff
  3. 1d noreplay is zero diff.

Why we need develop GEOSgcm_GridComp for this PR is because there is a bug and I believe @mathomp4 is working on some resolution. If we don't use develop CMake will fail due to No SOURCES given to target: w3adc. issue is here

@biljanaorescanin

This should now be taken care of in main. @sdrabenh released a new version of GEOSgcm_GridComp and added it to main and it now matches the newer WaveWatch3 version in GEOS.

@biljanaorescanin
Copy link
Contributor

Just a note for @sdrabenh:
I should just clarify GCM uses hefsurface as default and this will speed up GCM Surface for ~25% .
GEOSldas uses Louis for now so if I don't change default setting this PR is always trivially zero diff for GEOSldas.
From GEOS_SurfaceGridComp.rc:
# GEOSagcm=>CHOOSEMOSFC: 1
# GEOSldas=>CHOOSEMOSFC: 0

@mathomp4
Copy link
Member

mathomp4 commented Mar 3, 2025

Just a note for @sdrabenh: I should just clarify GCM uses hefsurface as default and this will speed up GCM Surface for ~25% . GEOSldas uses Louis for now so if I don't change default setting this PR is always trivially zero diff for GEOSldas. From GEOS_SurfaceGridComp.rc:
# GEOSagcm=>CHOOSEMOSFC: 1
# GEOSldas=>CHOOSEMOSFC: 0

Yep. I got this just wrong with @sdrabenh when talking with him.

So, I guess this is a rare win-win. Faster and zero-diff!

@gmao-rreichle gmao-rreichle added the Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Mar 11, 2025
@gmao-rreichle gmao-rreichle changed the title vectorize the operation in helfsurfce and other subroutines vectorize helfsurface() and related subroutines Apr 2, 2025
@gmao-rreichle gmao-rreichle removed the Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Apr 2, 2025
@mathomp4
Copy link
Member

mathomp4 commented Apr 2, 2025

Sigh. I need to update the CI in here as well...

@sdrabenh sdrabenh merged commit d5eb14c into main Apr 15, 2025
6 checks passed
@sdrabenh sdrabenh deleted the feature/wjiang/cleanup_helsurface branch April 15, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0 diff The changes in this pull request have verified to be zero-diff with the target branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants