Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove multi-grid wave support #3188

Merged
merged 35 commits into from
Jan 7, 2025
Merged

Remove multi-grid wave support #3188

merged 35 commits into from
Jan 7, 2025
+81 −328

Conversation

AntonMFernando-NOAA
Copy link
Contributor

@AntonMFernando-NOAA AntonMFernando-NOAA commented Dec 20, 2024 •
edited
Loading

Description

  • Eliminate instances where waveGRD is a list containing multiple supported wave grids. Also, remove the associated for loops (e.g., for wavGRD in ${waveGRD}; do)

  • Resolves Remove multi-grid wave support #2637

Type of change

  • Maintenance (code refactor, clean-up, new CI test, etc.)

Change characteristics

  • Is this a breaking change (a change in existing functionality)? NO
  • Does this change require a documentation update? NO
  • Does this change require an update to any of the following submodules? NO (If YES, please add a link to any PRs that are pending.)
    • EMC verif-global
    • GDAS
    • GFS-utils
    • GSI
    • GSI-monitor
    • GSI-utils
    • UFS-utils
    • UFS-weather-model
    • wxflow

How has this been tested?

  • CI tests with HERA and HERCULES

Checklist

  • Any dependent changes have been merged and published
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have documented my code, including function, input, and output descriptions
  • My changes generate no new warnings
  • New and existing tests pass with my changes

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 20, 2024
View reviewed changes
ush/parsing_namelists_WW3.sh Fixed Show fixed Hide fixed
ush/parsing_namelists_WW3.sh Fixed Show fixed Hide fixed
@AntonMFernando-NOAA AntonMFernando-NOAA marked this pull request as ready for review December 23, 2024 16:39
@AntonMFernando-NOAA AntonMFernando-NOAA removed the request for review from junwang-noaa December 23, 2024 16:39
aerorahul
aerorahul reviewed Dec 23, 2024
View reviewed changes
Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

Please see comments in the review.

This PR removes the waveMULTIGRID boolean option that enabled single/multiple wave grids defined under the variable waveGRD.
The scripts using this are split into if-else blocks. The elseblocks for a single grid waveMULTIGRID=.false. are very simple. However, the workflow has been exercising and developing on the if block of waveMULTIGRID=.true. even for cases where waveGRD is a single grid.
This PR rectifies that.
Careful examination and review is needed and appreciated from the wave experts to ensure the changes introduced in this PR does not impact (negatively) the wave grid operations.

Apart from a few suggestions, this PR looks good. Approve pending till after the wave team has a chance to review and approve the changes proposed in this PR.

ush/forecast_postdet.sh Show resolved Hide resolved
ush/parsing_namelists_WW3.sh Outdated Show resolved Hide resolved
ush/parsing_namelists_WW3.sh Outdated Show resolved Hide resolved
sbanihash
sbanihash reviewed Dec 23, 2024
View reviewed changes
Copy link
Contributor

@sbanihash sbanihash left a comment

Choose a reason for hiding this comment

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

Changes look good. Thank you @AntonMFernando-NOAA for your work on this. I have no comments on the changes, but would appreciate checking the wave history+products with this change before approving. Is there a test case you could share with me? If not I can make a quick test myself.

@AntonMFernando-NOAA
Copy link
Contributor Author

AntonMFernando-NOAA commented Dec 24, 2024 •
edited
Loading

Changes look good. Thank you @AntonMFernando-NOAA for your work on this. I have no comments on the changes, but would appreciate checking the wave history+products with this change before approving. Is there a test case you could share with me? If not I can make a quick test myself.

@sbanihash Test cases are here at HERA.
gfs: /scratch1/NCEPDEV/global/Anton.Fernando/RUNTESTS/EXPDIR/gfs_rw2
gefs: /scratch1/NCEPDEV/global/Anton.Fernando/RUNTESTS/EXPDIR/gefs_rw

sbanihash
sbanihash previously approved these changes Dec 24, 2024
View reviewed changes
Copy link
Contributor

@sbanihash sbanihash left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Wave outputs and history files look good. I approve this PR.
edit: I have noticed that you have added a path to gefs testing as well, but the global-workflow directories used for gfs & gefs do not seem to be the same. I did do a diff and the differences seem to be unrelated to the changes made in this PR.

@emcbot emcbot added CI-Hera-Passed **Bot use only** CI testing on Hera for this PR has completed successfully and removed CI-Hera-Running **Bot use only** CI testing on Hera for this PR is in-progress labels Jan 7, 2025
@emcbot
Copy link

emcbot commented Jan 7, 2025

CI Passed on Hera in Build# 4
Built and ran in directory /scratch1/NCEPDEV/global/CI/3188


Experiment C48mx500_3DVarAOWCDA_5b484f4d Completed 2 Cycles: *SUCCESS* at Mon Jan  6 22:10:13 UTC 2025
Experiment C48_ATM_5b484f4d Completed 2 Cycles: *SUCCESS* at Mon Jan  6 22:10:13 UTC 2025
Experiment C48mx500_hybAOWCDA_5b484f4d Completed 2 Cycles: *SUCCESS* at Mon Jan  6 22:10:37 UTC 2025
Experiment C96_S2SWA_gefs_replay_ics_5b484f4d Completed 1 Cycles: *SUCCESS* at Mon Jan  6 22:23:12 UTC 2025
Experiment C96_atm3DVar_5b484f4d Completed 3 Cycles: *SUCCESS* at Mon Jan  6 23:24:20 UTC 2025
Experiment C96C48_hybatmDA_5b484f4d Completed 3 Cycles: *SUCCESS* at Mon Jan  6 23:24:28 UTC 2025
Experiment C96C48_hybatmaerosnowDA_5b484f4d Completed 3 Cycles: *SUCCESS* at Mon Jan  6 23:39:06 UTC 2025
Experiment C96C48_ufs_hybatmDA_5b484f4d Completed 3 Cycles: *SUCCESS* at Tue Jan  7 00:08:08 UTC 2025
Experiment C48_S2SW_5b484f4d Completed 2 Cycles: *SUCCESS* at Tue Jan  7 00:20:33 UTC 2025
Experiment C48_S2SWA_gefs_5b484f4d Completed 1 Cycles: *SUCCESS* at Tue Jan  7 00:45:56 UTC 2025

@KateFriedman-NOAA KateFriedman-NOAA added CI-Wcoss2-Building **Bot use only** CI testing is cloning/building on WCOSS CI-Wcoss2-Failed **Bot use only** CI testing on WCOSS for this PR has failed CI-Wcoss2-Running **Bot use only** CI testing on WCOSS for this PR is in-progress and removed CI-Wcoss2-Building **Bot use only** CI testing is cloning/building on WCOSS CI-Wcoss2-Running **Bot use only** CI testing on WCOSS for this PR is in-progress labels Jan 7, 2025
@KateFriedman-NOAA
Copy link
Member

CI Tests set up to run in /lfs/h2/emc/ptmp/emc.global/PR/PR_3188/RUNTESTS on WCOSS

@WalterKolczynski-NOAA WalterKolczynski-NOAA removed the CI-Wcoss2-Failed **Bot use only** CI testing on WCOSS for this PR has failed label Jan 7, 2025
@KateFriedman-NOAA KateFriedman-NOAA added CI-Wcoss2-Building **Bot use only** CI testing is cloning/building on WCOSS CI-Wcoss2-Failed **Bot use only** CI testing on WCOSS for this PR has failed CI-Wcoss2-Running **Bot use only** CI testing on WCOSS for this PR is in-progress and removed CI-Wcoss2-Building **Bot use only** CI testing is cloning/building on WCOSS labels Jan 7, 2025
@KateFriedman-NOAA
Copy link
Member

CI Tests set up to run in /lfs/h2/emc/ptmp/emc.global/PR/PR_3188/RUNTESTS on WCOSS

@WalterKolczynski-NOAA WalterKolczynski-NOAA removed CI-Wcoss2-Failed **Bot use only** CI testing on WCOSS for this PR has failed CI-Wcoss2-Running **Bot use only** CI testing on WCOSS for this PR is in-progress labels Jan 7, 2025
@KateFriedman-NOAA KateFriedman-NOAA added CI-Wcoss2-Building **Bot use only** CI testing is cloning/building on WCOSS CI-Wcoss2-Running **Bot use only** CI testing on WCOSS for this PR is in-progress and removed CI-Wcoss2-Building **Bot use only** CI testing is cloning/building on WCOSS labels Jan 7, 2025
@KateFriedman-NOAA
Copy link
Member

CI Tests set up to run in /lfs/h2/emc/ptmp/emc.global/PR/PR_3188/RUNTESTS on WCOSS

@aerorahul
Copy link
Contributor

@JessicaMeixner-NOAA @sbanihash
Any final comments before we merge this PR after the WCOSS2 test finishes? (It likely will not since there was a production switch, @KateFriedman-NOAA you might have to either restart or bypass it) Would still like your final approvals now that Hera and Hercules tests have passed

aerorahul
aerorahul approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

lgtm

@JessicaMeixner-NOAA JessicaMeixner-NOAA mentioned this pull request Jan 7, 2025
Open
21 tasks
@aerorahul aerorahul requested a review from sbanihash January 7, 2025 15:45
sbanihash
sbanihash approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@sbanihash sbanihash left a comment

Choose a reason for hiding this comment

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

Changes look good. Thank you @AntonMFernando-NOAA for your work on this. Thank you @JessicaMeixner-NOAA for also reviewing the changes.

@KateFriedman-NOAA KateFriedman-NOAA added CI-Wcoss2-Passed **Bot use only** CI testing on WCOSS for this PR has completed successfully and removed CI-Wcoss2-Running **Bot use only** CI testing on WCOSS for this PR is in-progress labels Jan 7, 2025
@aerorahul aerorahul merged commit fececaf into NOAA-EMC:develop Jan 7, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbanihash sbanihash sbanihash approved these changes

@JessicaMeixner-NOAA JessicaMeixner-NOAA JessicaMeixner-NOAA approved these changes

@aerorahul aerorahul aerorahul approved these changes

@WalterKolczynski-NOAA WalterKolczynski-NOAA Awaiting requested review from WalterKolczynski-NOAA WalterKolczynski-NOAA is a code owner

Assignees

@AntonMFernando-NOAA AntonMFernando-NOAA

Labels
CI-Hera-Passed **Bot use only** CI testing on Hera for this PR has completed successfully CI-Hercules-Passed **Bot use only** CI testing on Hercules for this PR has completed successfully CI-Wcoss2-Passed **Bot use only** CI testing on WCOSS for this PR has completed successfully
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove multi-grid wave support
7 participants
@AntonMFernando-NOAA @emcbot @JessicaMeixner-NOAA @KateFriedman-NOAA @aerorahul @sbanihash @WalterKolczynski-NOAA