-
Notifications
You must be signed in to change notification settings - Fork 543
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
Add switch_meshcap_pdlib_IC4 and change dev/ufs-weather-model IC4M8 to IC4M10 #1211
base: dev/ufs-weather-model
Are you sure you want to change the base?
Add switch_meshcap_pdlib_IC4 and change dev/ufs-weather-model IC4M8 to IC4M10 #1211
Conversation
@NickSzapiro-NOAA thanks for submitting a fix to address this numbering conflict. Please stay tuned, more information to follow later today. |
@NickSzapiro-NOAA thanks for your patience, I just wanted to confirm there were not other parts of the code that needed updated before giving @MatthewMasarik-NOAA the thumbs up. I have done that and this is good to go. Thanks for proactively addressing the conflict and for your patience as we work through the merge from develop causing unexpected answer changes. |
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 looks good from my side.
Approved.
@NickSzapiro-NOAA I think you should also change the comments Lines 359 to 360 in d9b3172
|
@NickSzapiro-NOAA please merge in dev/ufs-weather-model to this branch. |
@NickSzapiro-NOAA apologies this is up to date with dev/ufs-waether-model |
@JessicaMeixner-NOAA @MatthewMasarik-NOAA we are enabling wave-sea ice coupling in existing cpld pdlib test cases rather than add a separate test case. The cpld_restart_pdlib_p8 test then fails in WW3 with: |
@NickSzapiro-NOAA Thank you for making us aware of this update and to you and @DeniseWorthen the fix! This type of change requires additional testing on the standalone wave side. I or @MatthewMasarik-NOAA will reply back to this thread when that is complete. We likely will be making a simultaneous PR to the develop branch so this fix is added there as well. |
@NickSzapiro-NOAA just an FYI that standalone testing has started. Should have an update later this afternoon or tomorrow morning. I've also started testing for cherry-picking the relevant change to add to the develop branch which can be seen here: https://github.com/JessicaMeixner-NOAA/WW3/tree/feature/cherrypickpr1211todev |
@NickSzapiro-NOAA since we have changed what is being written/read in the restarts, the restart file idenfication line: I do want to make you aware that this means this change will have a big impact as it means you have to have new restarts or initial conditions after this change is merged. That means we have to do some modifications to replay restarts and will impact the GEFS team reforecast efforts (@sbanihash @NeilBarton-NOAA @bingfu-NOAA). We can make scripts to update these restarts, but it's extra work required if model versions are updated. All HR3 restarts would no longer be valid for anyone to test, etc until we update those as well. This bug fix needs to be made, but I want to make everyone aware and potentially coordinate the timing with projects to minimize disruption as much as possible. |
Can I ask, how is WW3 standalone w/ ICE1 and ICE5 passing restart tests? |
@DeniseWorthen our standalone regression testing could use improvement and not all features are tested with restarts, its an area we hope to improve on when we have available resources to implement the updates. |
I see. I was more concerned that it was passing restart w/ standalone, but not coupled. |
To maintain compatibility with restarts in the mean time, is a workaround to set rather than read TIC1 and TIC5 instead on restart? Maybe to TICE? |
@NickSzapiro-NOAA that's a creative solution. Apologies for the delay, I needed to go refresh my memory. I think this would work in a coupled setting for your tests where all the inputs were at the same time,but in reality these should be allowed to be different times and the proper bug fix should be implemented. I'll work towards continued testing of this PR and will work early next week on the work-around for the GEFS team. It should be straight forward, it just requires testing and coordination and processing of a lot of IC files. |
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.
VERINI needs to be updated in w3iors before we move forward. Otherwise, the WW3 standalone regression tests performed as expected.
Thanks for updating VERINI. I'll rerun the standalone tests with this update to post that. I've also discussed with the GEFS team about how to update the restarts that they'll need and will be working on setting that up. |
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.
Thanks for updating the VERINI number and for your patience as I ran some standalone tests.
I had to cherry pick one bug-fix we have in develop to get the multi-grid tests to run (I forgot about this change), so it took a minute. We do have lots of diffs, but they're all in the restart files which is expected.
matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt
We are still in the process of creating the conversion utilities for the GEFS project and we'll also have to stage updated ICs for HRs as well. For now, we have a first draft of instructions on how this process is done: https://github.com/NOAA-EMC/WW3/wiki/Converting-Binary-Restarts-from-Older-to-Newer-Versions This process is still on going. @NeilBarton-NOAA @bingfu-NOAA or @sbanihash if you would like to have this PR be held until the utility to convert the restarts for GEFS has been fully tested please let @NickSzapiro-NOAA know.
Thank you for testing and coordinating, @JessicaMeixner-NOAA! Please let me know if you would like me to update the WW3 input directory for ufs-weather-model RTs |
There shouldn't need an additional update to the inputs, but if you run into any issues please let me know and I can help with creating new ones. |
@NickSzapiro-NOAA - just wanted to check in on this PR - if there's anything you need from us on the wave code management side, please let us know! |
There are two IC4M10 PRs to the emc/develop branch (#1293 and #1294) that will change the switch file and the contents of the method. One option is to work through those PRs first and then bring (the CESM and E3SM) wave-ice updates to dev/ufs-weather-model and UFS. Does this sound good? #1294 also raises the question of whether other IC4 schemes should use the IC4_ACCURATE_NUMERICS switch as well... |
@NickSzapiro-NOAA Thanks for this info. From the code management perspective we will look into (#1293 and #1294) this week and we can go from there! |
Pull Request Summary
Add switch_meshcap_pdlib_IC4 and change dev/ufs-weather-model IC4M8 to IC4M10 following #1176 to WW3:develop
Description
This PR adds a configuration to run IC4M10 on the unstructured grid in UFS Weather Model. No changes from new switch file. Renumbering to IC4M10 after #1176 added IC4M{8,9}
Issue(s) addressed
fixes #1187
needed for ufs-community/ufs-weather-model#1969
Commit Message
Add switch_meshcap_pdlib_IC4 and change dev/ufs-weather-model IC4M8 to IC4M10
Check list
Testing
Regression tests and operational requirement tests were run in UWM on Hera. See ufs-community/ufs-weather-model#2072