-
Notifications
You must be signed in to change notification settings - Fork 540
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
Bring CMEPS mediator compatibility to WW3 cap #396
Bring CMEPS mediator compatibility to WW3 cap #396
Conversation
@uturuncoglu can you update the manual (this is the relevant part: https://github.com/NOAA-EMC/WW3/blob/develop/manual/app/nuopc.tex) or at least create a new issue so that we can track that these updates need to be added to the manual at some point? |
@JessicaMeixner-NOAA I think that creating issue about the documentation is better idea since we did not finalize the code and it could change it slightly based on the test of the global application but if you want me to add section to the documentation that is also fine for me. |
@JessicaMeixner-NOAA I created an issue about the documentation. Here is the link #399 |
I ran the full regtest on hera with expected known non-b4b tests
|
The regression tests that will matter most for this is to test the ufs-weather-model. However, we should probably wait for the WW3 PR that updates the code before running those. @danrosen25 @BinLiu-NOAA anyone else want to review this code? I have done my first quick review and regression tests for previous ufs-weather-model passed, but they should be reviewed and I should probably do a more in-depth review as well. |
@JessicaMeixner-NOAA I think we should associate a milestone to this development on github and define the version accordingly. |
@aliabdolali sounds good. @danrosen25 @BinLiu-NOAA @uturuncoglu @aliabdolali I'm going to set up a meeting to do a group review of this code. |
Here is the milestone |
@@ -1750,6 +1916,9 @@ | |||
! -------------------------------------------------------------------- / | |||
! 1. Check that required import fields show correct time stamp | |||
! | |||
if (med_present) then | |||
allUpdated = .true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This by-passes the import data-dependency. The import data-dependency restarts winds from an external component. During CMEPS restart runs this DataInitialize subroutine should check imported data time stamp to determine if CMEPS has restarted winds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to revisit this part once we have a configuration that restarts the model and uses CMEPS for coupling. I think there is no need to take action at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be a fairly major issue here. If it's not addressed now, there needs to be an issue documenting that this is happening and that we need to look into it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JessicaMeixner-NOAA new issue - #428 - is created to track it.
model/ftn/wmesmfmd.ftn
Outdated
! 2.h Set ESMF import grid corner coordinates, only for coupled with mediator | ||
! | ||
if (med_present) then | ||
call ESMF_GridAddCoord( impGrid, & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check to make sure we can't get to this section of code w a unstructured grid and we need to have a if GTYPE if statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked and we only get to this routine for the select GTYPES, so no need for a if GTYPE statement:
if ( (GTYPE.eq.RLGTYPE).or.(GTYPE.eq.CLGTYPE) ) then
write(msg,'(a)') trim(cname)// &
': Creating import grid for Reg/Curvilinear Mode'
call ESMF_LogWrite(trim(msg), ESMF_LOGMSG_INFO)
call CreateImpGrid( gcomp, rc=rc )
if (ESMF_LogFoundError(rc, PASSTHRU)) return
elseif (GTYPE.eq.UNGTYPE) then
write(msg,'(a)') trim(cname)// &
': Creating import mesh for Unstructured Mode'
call ESMF_LogWrite(trim(msg), ESMF_LOGMSG_INFO)
call CreateImpMesh( gcomp, rc=rc )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related with the adding corner coordinates? I am asking because there is no such check in the code that adds corner coordinates.
@uturuncoglu thanks for being patient with me as I understand all of this better. I was able to access your directory and will look at the results more closely today which I think will help me see what you have commented here. |
@JessicaMeixner-NOAA Sure. Let me know if you find anything that needs to be fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made one more comment noting an area where we can improve on efficiency if needed in the future. Since both WW3 and ufs-weather-model have updated since the last time the ufs-weather-model regression tests were run using this branch, we should probably do that one more time before merging. Otherwise, I think this is good to go. Thanks Ufuk and sorry for the delay in my review.
@uturuncoglu I ran the ufs-weather-model regtests w/your branch (after merging in WW3 develop) and the regression tests did not pass. I didn't get a chance to figure out why, but wanted to let you and @aliabdolali know so we can look into that before merging this PR. |
@JessicaMeixner-NOAA as remember correctly, it was passing before right? I also remember that I run full RTs and it was passing. We have following possibilities:
Let me look at those and I'll update you about it. |
Sorry, I closed and reopen the PR. It is my fault. |
@JessicaMeixner-NOAA I run the
This is the line of code that perform halo update,
So, it does not makes sense to me. Anyway, I'll try to roll back my recent changes to see what happens. The only think that is in my mind is the commit that removes control to add corner coordinates to import and export grid. Anyway, i'll update you about it. |
I go back just before commit that removes the check for adding corner coordinates and model seems working and this particular test passes. So, it seems that hafs-community@f492972 commit has the issue if we run it with connectors. At this point I am not sure why it is failing. Maybe halo region is not defined correctly in corner coordinates or maybe it is an issue in ESMF side. I'll check and let you know. |
@JessicaMeixner-NOAA I think I solved the issue. Please see following commit 1f1ed07. I also run |
@JessicaMeixner-NOAA I run all wave related regression tests (cpld_bmark_wave, cpld_bmarkfrac_wave, cpld_bmarkfrac_wave_v16 and cpld_control_wave) and all of them were passed on Orion. I also tested fully coupled FV3+WW3 regional configuration (two-way interaction) under HAFS applications and it is also working without any issue. |
@uturuncoglu thanks for tracking down and solving the issue. I'll retest on hera today to confirm your results on orion. |
After hera maintenance, I ran the full ufs-weather-model regression tests with hera.intel with this branch of WW3 + merging in the develop branch and one test failed: |
re-running the failed control_atm_aerosols test, it passed this time w/hera.intel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A group review of this code was completed and outstanding items were either addressed or issues for follow up were made.
The ufs-weather-model (using commit hash b64e648c34bd177a761195fe0bf17da6be44fc09) + this branch with the WW3 develop merged in passed all regression tests for me on hera.intel
Thanks @uturuncoglu for all of the detailed explanations and these updates!!!!
@JessicaMeixner-NOAA that is great news. I think that PR is in better shape now with your comments and suggestions. |
The regtest passed successfully on hera using intel,impi, with expected known non-b4b cases:
|
@ danrosen25 could you provide your review, so we can move on and merge this PR. |
@danrosen25 do you have anything that needs to be addressed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't been able to run regression tests but I'm holding up the completion of this PR. I am not keen on the use of med_present as this creates an end-user configuration that needs documentation for a package of features that seem to be independent on med_present=t/f. There are still a few things that will need to be addressed such as data dependencies during initialization
@danrosen25 @uturuncoglu are we in such a rush that we need to merge things we aren't okay with? At minimum can we make an issue to address the "if med_present" issue? What are the alternatives for getting rid of this if- mediator statements? We currently have an issue for the initialization: With not running the regression tests, was this a time issue or a the regression tests aren't working issue? |
@JessicaMeixner-NOAA I think that we already remove most of |
@JessicaMeixner-NOAA currently
So, we don't have too much related with |
@JessicaMeixner-NOAA |
Sounds good.. Note that there needs to be a separate ufs-weather-model PR for this or you can work with @aliabdolali as he has an open ufs-weather-model PR that involves updating WW3 to the top of develop that is not yet merged |
Pull Request Summary
This PR aims to bring capability to run WW3 wave component through the use of CMEPS mediator. The initial implementation is done as a part of HAFS application and supports ATM-WAV and ATM-OCN-WAV (with HYCOM) configurations.
Description
This PR adds following,
mediator_present = .true.
needs to be added toWAV_attributes
to use CMEPS mediator.wind.ww3
). The special subroutine in the cap reads the forcing file and merge with the data coming from coupling to provide complete set of wind field to WW3. The merge mask is also generated on the fly by checking fill value of the fields coming from CMEPS mediator. This is also controlled by extra attributemerge_import = .true.
inWAV_attributes
and it is not required for the global application.Issue(s) addressed
WW3 can be used with CMEPS mediator to exchange data among the components.
Check list
Testing
Full UFS Weather Model RTs were run including existing configurations with wave coupling
There is no change in the regression tests
N/A
Yes, full regression tests were run without any issue on Orion and Hera.
Please indicate the expected changes in the outputs (excluding the known list of non-identical tests).