-
Notifications
You must be signed in to change notification settings - Fork 160
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
Adding I/O interfaces in GSI for Analysis of Significant Wave Height (HOWV) and near-surface Wind Gust (GUST) for WRF-ARW based 3DRTMA #835
base: develop
Are you sure you want to change the base?
Conversation
ARW-ARW model forecast as firstguess.
firstguess file, then the execution of code is aborted, instead of just printing out the error message.
…avhgtwndgst_wrfarw
@GangZhao-NOAA Thanks. Your inline comments are good and I just wondered if there are already existing options for user to do the inflation. If there is no, that is ok. That comment will help users/developer understand the situation. |
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 continual enhancements of GSI
@TingLei-NOAA Hi Ting, I realized that I should not use the word "inflat" here, since "covariance inflation" is somehow a specific term used in ensemble DA. I will change it to "increase" in my next PR. Since the process to increase the static error for HOWV and GUST are done outside of GSI code, not in GSI, so I'd better remove these comments in my next PR. Thank you again! |
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.
Did not test code. Most comments are minor.
Question: Do any of the existing GSI ctests exercise the functionality added by this PR? If not, (a) how do we know the changes work as intended? and (b) what's the strategy to ensure future PRs do not break the functionality added by this PR?
…avhgtwndgst_wrfarw
@RussTreadon-NOAA
This code was tested with a real case data (2024-12-12_00z)
As you pointed out, NONE of the existing GSI ctests exercises the functionality added by this PR.
This code was tested with a real case data (2024-12-12_00z) with the added functionality (analyzing wave height and wind gust in hybrid 3DEnVar with HRRR-based first guess). And it passed all the 6 existing ctests (regression test) in current GSI package.
Thank you for pointing out this question! I discussed with Manuel (@ManuelPondeca-NOAA). We should add a new ctest item for the new functionalities added for 3DRTMA. We are busy with current HRRR-3DRTMA workflow project, so we cannot provide such kind of ctest for now. We will do it in the near future. |
minor changes suggested by code reviewers. With version control on github along with well documented issues, added changelog entries to source code is no longer needed.
@guoqing-noaa @TingLei-NOAA @ShunLiu-NOAA @RussTreadon-NOAA Following your questions/suggestions/comments, I modified the code and re-submitted the PR #835 for review. Most of the updated part are in the comments, so there should be no essential differences, comparing to the previous commit. If you have time, could you please make another round code review on these newly-updated code? I am running all the 6 existing ctests of GSI on Hera now, and will run ctests on WCOSS2 very soon this afternoon. Then I will update the new reports from the ctests here. Thank you very much for your help! -Gang |
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.
Thank you @GangZhao-NOAA for updating the code. Changes look good.
While this PR is under review, the GSI main branch (develop) was updated to commit #8e54642. So I merged that latest update into my code, and then following the suggestions & comments from reviewers, made some modifications to the code. Now this PR #835 is to merge my latest commit #94bed49 (in branch "feature/rtma3d_wavhgtwndgst_wrfarw") to EMC-GSI. Due to these new changes to the code, I re-ran the ctests on Hera and WCOSS2 (Cactus):
Here is the reports of the ctest (regression tests) running on WCOSS2 (Cactus):
|
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.
Ctests pass on WCOSS2 and Hera.
Approve.
Russ, Thank you for running ctest on WCOSS2 and Hera. |
@TingLei-NOAA Could you review the new changes of this PR? |
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.
The issues related to code documentation and potentially excessive print calls have previously been resolved. Approve.
Description
Motivation:
Method:
Related Issues:
Resolves #834
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist
Here is the reports of the regression tests on WCOSS2 (Dogwood):
Here is the reports of the regression tests on Hera: