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.

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

Modifications for ufs-coastal app #2396

Open
wants to merge 51 commits into
base: develop
Choose a base branch
from

Conversation

DeniseWorthen
Copy link
Collaborator

@DeniseWorthen DeniseWorthen commented Aug 12, 2024

Commit Queue Requirements:

  • Fill out all sections of this template.
  • All sub component pull requests have been reviewed by their code managers.
  • Run the full Intel+GNU RT suite (compared to current baselines) on either Hera/Derecho/Hercules
  • Commit 'test_changes.list' from previous step

Description:

Updates WW3 for modifications and fixes required by UFS-Coastal App

Commit Message:

* UFSWM - adds additional compile switch and namelist changes required for WW3 in Coastal App
  * WW3 - add export fields required for coupling with SCHISM

Priority:

  • Normal (but PR has been held since late summer due to PR pause in WW3 repo)

Git Tracking

UFSWM:

Sub component Pull Requests:

UFSWM Blocking Dependencies:

  • None

Changes

Regression Test Changes (Please commit test_changes.list):

  • PR Updates/Changes Baselines.

None

Input data Changes:

  • None.

Library Changes/Upgrades:

  • Required
    • Library names w/versions:
    • Git Stack Issue (JCSDA/spack-stack#)
  • No Updates

Testing Log:

  • RDHPCS
    • Hera
    • Orion
    • Hercules
    • Jet
    • Gaea
    • Derecho
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
  • opnReqTest (complete task if unnecessary)

DeniseWorthen and others added 2 commits August 8, 2024 18:45
* the wave restarts are not b4b against the develop baseline
but are b4b when compared against a baseline generated with the
trho fix

rt_atmwav_control_noaero_p8_intel.log: Comparing ufs.atmw.ww3.r.2021-03-22-64800 .....USING CMP......NOT IDENTICAL
rt_hafs_regional_atm_wav_intel.log: Comparing ufs.hafs.ww3.r.2019-08-29-21600 .....USING CMP......NOT IDENTICAL
@DeniseWorthen DeniseWorthen changed the title modifications for ufs-coastal app Modifications for ufs-coastal app Aug 12, 2024
@DeniseWorthen DeniseWorthen marked this pull request as ready for review August 12, 2024 23:08
@uturuncoglu
Copy link
Collaborator

@DeniseWorthen Do you think that we need to find more meaningful aliases for the new fields defined in the dictionary. We could try to come up a set of standard names for them but not sure. Let me know what you think?

@DeniseWorthen
Copy link
Collaborator Author

@uturuncoglu I'm not sure how useful the aliases are if we have the description. So, for example we could make an alias for Sw_ubry but it would basically be the description, something like alias: near_bottom_rms_wave_velocities_y? Do you know if SCHISM for example would prefer to import a field using a descriptive alias?

I do see now I should have probably named some of the new fields with "Fw" (as fluxes) and not "Sw" as states.

@uturuncoglu
Copy link
Collaborator

@DeniseWorthen Yes, SCHSIM follows convention like that. See following part, https://github.com/oceanmodeling/schism-esmf/blob/12e2ce43475fe85212c60de7894524d22a1bab96/src/schism/schism_nuopc_cap.F90#L414. It is not big deal and I am using short names at this point for the implementation but we might consider to add aliases and use them in the SCHSIM side. anyway, I'll let you know when PR is ready in ufs coastal side. So, you could check it.

@DeniseWorthen
Copy link
Collaborator Author

I see, thanks. We definitely might need aliases then.

@uturuncoglu
Copy link
Collaborator

@DeniseWorthen @JessicaMeixner-NOAA @janahaddad We had a meeting with costal team today. Here is our proposal,

  • Let's keep this PR and also WW3 one active for now. So, it might be merged with the UFS WM around November time frame (waiting in freeze mode 2-3 months) once WW3 team have more resource. We have no rush at this point and we could still test the implementation in our end without any issue.

  • In the mean time, I'll create a new branch in our UFS WM fork to bring all these changes. This will allow us to have further test in vortex coupling with a new configuration (we are working on it in Coastal side) and also create new RT to test it regularly. It will also help us to find issues in the coupling if we have and then we could fine tune the PR in here if it is required.

  • Then, once top level UFS WM PR is merged, we could sync UFS Costal fork to get all the changes.

Anyway, let me know what do you think about this plan. We are open any other suggestion at this point.

@DeniseWorthen
Copy link
Collaborator Author

@uturuncoglu I do think it is a good idea for Coastal to test the changes in the WW3 cap as much as possible while we wait for the stoppage of PRs in WW3 to lift. Also keep in mind that once the stoppage lifts, PRs to dev/ufs-weather-model will most likely be further delayed as community development (going to the develop branch) will also need to proceed.

Also, I wanted to make Coastal aware of Issue NOAA-EMC/WW3#1298 in case you haven't seen it.

@uturuncoglu
Copy link
Collaborator

@DeniseWorthen That is great. BTW, Thanks for pointing the PIO PR.

* run on hercules due to disk quota issues on hera
@DeniseWorthen DeniseWorthen added the No Baseline Change No Baseline Change label Jan 11, 2025
@DeniseWorthen
Copy link
Collaborator Author

This PR is ready and now contains no baseline change since binary restart are no longer used for any UWM test. It has been held since late summer due to PR pause in WW3.

@jkbk2004
Copy link
Collaborator

@DeniseWorthen Go ahead to sync up. We can start working on this pr. @FernandoAndrade-NOAA @BrianCurtis-NOAA FYI

@DeniseWorthen
Copy link
Collaborator Author

Thanks. It should be ready to combine @uturuncoglu

@uturuncoglu
Copy link
Collaborator

@JessicaMeixner-NOAA @DeniseWorthen @jkbk2004 I am not sure about the latest decision but it is good to me to combine with the land PR. Let me know if you need anything from my end.

@jkbk2004
Copy link
Collaborator

@uturuncoglu I think it depends on the scope of baseline changes expected from this pr. @DeniseWorthen This PR only to support coastal app, right? No impact to existing baseline tests, right?

@uturuncoglu
Copy link
Collaborator

@jkbk2004 Sure. No worries. Just decide what you think best. Both works for me. Thanks again for your help.

@DeniseWorthen
Copy link
Collaborator Author

I tested this PR last week on Hercules (disk quota issues on Hera) and there were no baseline changes. This PR merges into the mesh cap fields which the CoastalApp depends on, but are unused currently in UFS.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Jan 17, 2025

I tested this PR last week on Hercules (disk quota issues on Hera) and there were no baseline changes. This PR merges into the mesh cap fields which the CoastalApp depends on, but are unused currently in UFS.

@DeniseWorthen Thanks for confirming! It's better to combine with #2556 since both PRs are no baseline change.

@jiandewang
Copy link
Collaborator

That will be great. I can push dev/EMC restart feature back to MOM main repo. once this is merged to dev/emc. NCAR is waiting for me to issue their PR.

@jkbk2004
Copy link
Collaborator

@DeniseWorthen
Copy link
Collaborator Author

Hold on, I need to add the changes in the mom6_files.cmake

@DeniseWorthen
Copy link
Collaborator Author

@jiandewang Please check.

@jiandewang
Copy link
Collaborator

@DeniseWorthen Just checked, MOM6 is pointing to the right branch and mom cmake list is using the updated one. Thanks.

@jkbk2004 jkbk2004 added the Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked. label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Baseline Change No Baseline Change Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants