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

Changes to enable coupling with WW3 using 3d vortex formulation #32

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

uturuncoglu
Copy link
Contributor

The WW3 changes are merged with UFS WM level PR: ufs-community/ufs-weather-model#2396. These are the remaining changes in the SCHISM side to enable coupling with 3d vortex formulation. This is still draft and before merging it, we need to following,

If everything goes as expected, I could make this PR ready for review. JFYI, @janahaddad

@josephzhang8
Copy link
Member

Thx @uturuncoglu.
@danishyo @platipodium: could you please review? Thx

@uturuncoglu
Copy link
Contributor Author

uturuncoglu commented Jan 28, 2025

@josephzhang8 JFYI, I have just synced UFS coastal with UFS WM. This also brings changes related to WW3. I am testing now by running RTs. Along my testing I also tried to update SCHISM but RTs are started to fail with following error.

10:    2: ABORT: INIT: USE_ATMOS must use nws=4

At this point we are using nws = 2 for our regression tests along with -DUSE_ATMOS=ON. I wonder if we could solve it by simply updating our param.nml for the recent version of the SCHISM.

@josephzhang8
Copy link
Member

Yes, you can simply change nws=4. Let me know if you still have errors. Thx

Copy link
Member

@platipodium platipodium left a comment

Choose a reason for hiding this comment

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

The code looks fine to me, thanks for adhering to the conventions set in the code base. I am not so happy with the naming of the variables 'Sw_*', but that can be fixed at a later stage and might be a consequence of the coupling. Please go ahead and merge.

Copy link
Member

@platipodium platipodium left a comment

Choose a reason for hiding this comment

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

Thanks for making the naming changes, as well. Please go ahead and merge. We might have to think about auto-generating documentation while we expand the field dictionary....

@platipodium platipodium marked this pull request as ready for review January 29, 2025 08:36
@josephzhang8 josephzhang8 merged commit d3ab56d into schism-dev:master Jan 29, 2025
@josephzhang8
Copy link
Member

Thx @platipodium. I've merged and closed the PR.

@uturuncoglu
Copy link
Contributor Author

uturuncoglu commented Jan 29, 2025

@josephzhang8 Again. this was a draft PR and I supposed to make it ready to review and then merge it after testing. I think merging PRs without testing is not good practice. It is just my feeling. @platipodium We could change the field naming since field dictionary allow us to define aliases.

@platipodium
Copy link
Member

I agree with Ufuk, first test it!. It was by accident that I converted from draft PR to PR ... always discovering new pitfalls when on GitHub

@uturuncoglu
Copy link
Contributor Author

@josephzhang8 @platipodium Okay. I have already found an issue with this PR. If you try to compile ESMF interface without building model with USE_WW3 option. The model is failing. I think we need to add this CPP flag also to ESMF interface. Anyway, the fix will come soon.

@uturuncoglu
Copy link
Contributor Author

@josephzhang8 @platipodium Okay. there are two solution for this issue.

  1. We could remove USE_WW3 from the get_ww3_arrays routine in the model since compute_wave_force_lon has no that flag. But, this also related to the data structures. It seems that data structures are defined in src/Core/schism_glbl.F90 without using USE_WW3. So, it would be safe to remove it from the model.
  2. In this solution, rather then removing USE_WW3 from the model, we could introduce it to ESMf interface around the SCHISM_StateImportWave3dVortex call.

Anyway, I just wonder about your preference. If you want to go with (2) then I think also introducing USE_WW3 to compute_wave_force_lon would be more consistent since that one also related with wave.

@josephzhang8
Copy link
Member

Option 1 is fine. I added that CPP to make the code clear but it's not needed as you mentioned. Thx

@uturuncoglu
Copy link
Contributor Author

@josephzhang8 Okay. Let me try. I'll update you about it.

@josephzhang8
Copy link
Member

josephzhang8 commented Jan 29, 2025

Sorry, I mixed this with another PR discussion.

@yunfangsun
Copy link

@uturuncoglu
Copy link
Contributor Author

uturuncoglu commented Jan 30, 2025

@yunfangsun I'll check it and let you know. I think I forgot to merge the CMEPS branch. Thanks for reminder.

@yunfangsun
Copy link

Hi @uturuncoglu ,

My /work2/noaa/nosofs/yunfangs/ufs-3d/ufs-weather-model_cdep_01052025/CMEPS-interface/CMEPS/mediator/esmFldsExchange_coastal_mod.F90 on Hercules is the most updated version.

Best

@uturuncoglu
Copy link
Contributor Author

@yunfangsun Okay. It is done. Please that with the recent version of the UFS coastal. BTW, do not forget to add new PIO options for the WW3 (an example is in here - https://github.com/oceanmodeling/ufs-weather-model/blob/711f55369d880c32548465a2443b4f31bc468df8/tests/parm/ufs.configure.coastal_datm_ocn_wav.IN#L57). You might also need to create mod.def from scratch using ww3_grid tool. I did it for the RTs. Let me know if you have any issues.

@yunfangsun
Copy link

Hi @uturuncoglu

Thank you! I will try it

@yunfangsun
Copy link

Hi @uturuncoglu ,

My DOCN is located at /work2/noaa/nosofs/yunfangs/ufs-3d/ufs-weather-model_cdep_01052025/CDEPS-interface/CDEPS/docn . Could you please take a look at it? Thank you!

@uturuncoglu
Copy link
Contributor Author

@yunfangsun did you modified docn_datamode_copyall_mod.F90?

@uturuncoglu
Copy link
Contributor Author

@yunfangsun Okay. I found it. I have just pushed the mods to CDEPS and updated UFS Coastal. Could you try. I did not push the change related with the So_omask since it also breaks other tests. If you could have issue with it we could look at it.

@yunfangsun
Copy link

Hi @uturuncoglu ,

Thank you! I will try it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants