-
Notifications
You must be signed in to change notification settings - Fork 151
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 pm2.5 DA for rrfs-smoke #482
add pm2.5 DA for rrfs-smoke #482
Conversation
c0969b6
to
5572c26
Compare
@hongli-wang I'm going to be starting on the review for this (sorry it has taken so long). Can you merge in 'develop' to your branch and we will get started. Do you know of anyone who knows GSI and AQ well enough to be a second reviewer besides myself? |
Do you mean rebase my feature/RRFS-CMAQ branch to the EMC GSI develop branch? What I see now is that my branch doesn't have conflicts and can merge to EMC GSI develop branch. Thanks for reviewing my GSI code. The others I can recommend are Ting Lei, Ming Hu and Mariusz Pagowski. Ting reviewed my previous update on RRFS-CMAQ DA, so if he would like to review the code this time would be best. |
@TingLei-daprediction would you have time to be able to provide a review of this PR alongside my review? |
@hongli-wang yes I see there are no conflicts so it should be fine (at least for now). I will proceed with starting my review. |
@CoryMartin-NOAA Sure. I will begin my review as soon as possible. |
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 couple of comments here from the first round of review.
@hongli-wang Have you finished the existing regression tests for this PR? I know they are only available on hera now. |
Do you know where I can find guidance on the regression test? I haven't done that. |
@hongli-wang @TingLei-NOAA , I/we (the GSI review team) will handle the regression testing, but I wanted to wait until it was closer to the 'final' version of the PR before submitting the tests. |
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 am not sure why my replies are shown as comments.
Hi Corry, Thanks for your comments. I will address them in my latter commit. |
No need to close this PR, we can keep it open for subsequent commits |
Understand. Thank you! |
|
@cory Martin - NOAA Federal ***@***.***> When some developers
have no access to hera, the reviewers take care of the regression tests is
essential . However, for those with access to hera, the regression
tests would be a necessary tool for their development , even before they
make their PR , not only as a criterion to pass but also as a helping tool
. In fact, i would think it would be helpful if every PR could
offer their own regression test to verify their new functions to the
existing regression tests, though this seems not doable in the near
future. So, when as you stated, the reviewing side would take care of the
regression tests, I would still open an issue with the link to GSI wiki on
how to do the regression tests and add my update on current practice on
hera to provide the options for GSI developers on hera.
…______________________________
Ting Lei
Physical Scientist, Contractor with Lynker in support of
EMC/NCEP/NWS/NOAA
5830 University Research Ct., Cubicle 2765
College Park, MD 20740
***@***.***
301-683-3624
On Tue, Dec 6, 2022 at 1:47 PM Cory Martin ***@***.***> wrote:
@hongli-wang <https://github.com/hongli-wang> @TingLei-NOAA
<https://github.com/TingLei-NOAA> , I/we (the GSI review team) will
handle the regression testing, but I wanted to wait until it was closer to
the 'final' version of the PR before submitting the tests.
—
Reply to this email directly, view it on GitHub
<#482 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/APEFS7DPYTHIZHPLOSSYG4LWL6C5RANCNFSM6AAAAAAQ4ZE2IE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
More developers can run regression test suite, the better for code development and review. We should update wiki page on how to run regression test as the steps may already be changed, at least they are not matching the steps I did. |
fb7d5bf
to
dfef936
Compare
@CoryMartin-NOAA @TingLei-daprediction Hi Cory and Ting, I had pushed back my changes. Would you please review the codes again? Thanks again for your suggestions and comments! Hongli |
@hongli-wang thank you for addressing my comments! I'm not sure I'll be able to get to do the regression tests before I go on leave, but will do so in the new year. Once we know those pass + 2 thumbs up reviews, we can proceed with the merge. |
@hongli-wang , once requested changes are made and your regression tests (ctest) results are posted I can approve this PR. |
@RussTreadon-NOAA I had changed the variables (iuv,ndynvario3d,ntracerio3d) to i_kind in gsi_rfv3io_mod.f90, and deleted unused commented variable (penalty) in intpm2_5.f90. The changes had been pushed back. |
Changes look good. Thank you @hongli-wang . Would you please post results from your execution of the regression (ctest) tests? @TingLei-NOAA , you have several unresolved conversations in this PR. Would you please check these open conversations and mark them as resolved if appropriate. |
@russ Treadon - NOAA Federal ***@***.***> Sure, all those
conversations I initiated were resolved. However, I think I don't have the
permission to mark those conversations resolved. I think you (with the
write permissions to EMC GSI ) or @hongli-wang (the author) could do it.
So, either of you do that will be ok to me and thanks !
…______________________________
Ting Lei
Physical Scientist, Contractor with Lynker in support of
EMC/NCEP/NWS/NOAA
5830 University Research Ct., Cubicle 2765
College Park, MD 20740
***@***.***
301-683-3624
On Tue, Jan 24, 2023 at 11:18 AM RussTreadon-NOAA ***@***.***> wrote:
Changes look good. Thank you @hongli-wang <https://github.com/hongli-wang>
. Would you please post results from your execution of the regression
(ctest) tests?
@TingLei-NOAA <https://github.com/TingLei-NOAA> , you have several
unresolved conversations in this PR. Would you please check these open
conversations and mark them as resolved if appropriate.
—
Reply to this email directly, view it on GitHub
<#482 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/APEFS7CXTVUSGZF2EE4JO6LWT76G5ANCNFSM6AAAAAAQ4ZE2IE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@RussTreadon-NOAA The regression test under my Hera account doesn't work well. FYI. I do change the regression_var.sh. I am wondering if it is the HPC setup issue. Here is the first regression test if you like to have a look. It seems the gsi.x (both develop and mine) cann't handle the namlist file: |
@TingLei-NOAA , did you try resolving the conversations by clicking on |
@hongli-wang , if you want me to examine this failure I need to know the path to the build directory from which you executed |
@RussTreadon-NOAA Thanks for looking into this. |
@TingLei-NOAA @RussTreadon-NOAA |
@hongli-wang , it looks like you did not include Log file
@hu5970 , step 1 of |
@hongli-wang Thanks for marking them resolved. |
Thank you @TingLei-NOAA for resolving the conversations you initiated. Please do so in future reviews. The review team can not process a PR if conversations remain open. |
@RussTreadon-NOAA You are correct. I did not include |
@RussTreadon-NOAA As I mentioned , those conversations could only be "resolved" by persons with write permission and the author, and hence, I have no "right" to "resolve " conversations. (this time , Hongli resolved those conversations. ). Hope this explanation make sense to you. |
@TingLei-NOAA . I searched github documentation and see what you are saying. It's always good to learn new things. Thanks. |
@RussTreadon-NOAA Thanks for the confirmation. Sure, that is also what I learned not long ago!. Thanks. |
@RussTreadon-NOAA @CoryMartin-NOAA My regression test is pending and no progress. Just wondering if any of you can run a regression test. Thanks. |
@hongli-wang , let me try later tonight. |
ctests work for me
My
|
@TingLei-daprediction , @TingLei-NOAA would you please add your review to this PR. @hongli-wang , any luck with the regression tests? |
@RussTreadon-NOAA The test of global_enkf failed. Others passed. 6/9 Test #2: [=[global_4dvar]=] ............... Passed 2044.37 sec 89% tests passed, 1 tests failed out of 9 Total Test time (real) = 2510.74 sec The following tests FAILED: My build on Hera: Please feel free to have a look. Thanks again. |
@hongli-wang , a check of file
The difference is 0.05 second. This is a non-fatal fail. Given @CoryMartin-NOAA and @TingLei-NOAA approvals, I'll merge this PR into |
Thanks @RussTreadon-NOAA for helping finish this up, I wanted a second set of eyes given that I contributed through the merging of develop + modifying the |
@CoryMartin-NOAA @TingLei-NOAA @RussTreadon-NOAA @ShunLiu-NOAA @hu5970 Thank you ALL for your time and contributions! Appreciated! Hongli |
Add PM2.5 DA capability for RRFS-SMOKE.