-
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
GitHub Issue NOAA-EMC/GSI#540 Modify to assimilate radar reflectivity and conventional data simultaneously without side effects in EnVar #543
Conversation
Is it possible to ask @chunhuazhou and @daviddowellNOAA to review this PR? I think they are well-qualified because they are experts of radar reflectivity assimilation. |
@shoyokota , attempts to add @daviddowellNOAA as a reviewer for this PR failed. This may be due to the fact that he is not in the NOAA-EMC organization. |
@chunhuazhou <https://github.com/chunhuazhou> is also not in NOAA-EMC.
Also, I remember that David with ID daviddowellNOAA
<https://github.com/daviddowellNOAA> made comments in PR504.
…On Wed, Feb 22, 2023 at 4:09 PM RussTreadon-NOAA ***@***.***> wrote:
@shoyokota <https://github.com/shoyokota> , attempts to add
@daviddowellNOAA <https://github.com/daviddowellNOAA> as a reviewer for
this PR failed. This may be due to the fact that he is not in the NOAA-EMC
organization.
—
Reply to this email directly, view it on GitHub
<#543 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACD5GZZZBCIJXSVBWX6U2K3WYZ6BNANCNFSM6AAAAAAVDRPDOQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
@daviddowellNOAA, you should receive an invitation to join NOAA-EMC. If you want to review this PR, please accept the invitation and I'll add you as a reviewer. |
@shoyokota Could you please email @daviddowellNOAA and ask if he is available to review this PR? Otherwise, we will have to find a new reviewer. |
@shoyokota Could you please contact terra.ladwig@noaa.gov to review this PR? |
src/gsi/gsimod.F90
Outdated
! If reflectivity is intended to be assimilated, beta_s0 should be zero. | ||
if ( beta_s0 > 0.0_r_kind )then | ||
! skipped in case of direct reflectivity DA because it works in Envar and hybrid | ||
if ( l_use_rw_columntilt .or. l_use_dbz_directDA) then | ||
do i=1,ndat | ||
if ( if_model_dbz .and. (index(dtype(i), 'dbz') /= 0) )then | ||
write(6,*)'beta_s0 needs to be set to zero in this GSI version, when reflectivity is directly assimilated. & | ||
Static B extended for radar reflectivity assimilation will be included in future version.' | ||
call stop2(8888) | ||
end if | ||
end do | ||
end if | ||
end if | ||
|
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.
Why is this removed? Is the current static B extended for radar reflectivity assimilation?
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 removal is simply to prevent from stopping radar reflectivity DA in the setting of hybrid EnVar.
The static B is not extended for reflectivity DA for now, so the static B for reflectivity is regarded as zero in the setting of hybrid EnVar. This is not necssarily the best for reflectivity DA, but zero static-B (ignoring the impact of reflectivity DA in the static-B part) itself is not so strange setting. In addition, the influence of zero static-B is not so large in case that the weight of ensemble B is much larger than that of static B.
Extending static B for reflectivity is defenitely important but it is not included in 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.
Since static B is not extended for reflectivity DA yet, assimilating reflectivity with static B in the hybrid EnVar doesn't sound right scientifically, even though the impact might not be that big. Do you have test figures to show the difference, say beta_s0=0.15 vs beta_s0=0 when only assimilating reflectivity in the hybrid EnVar?
And if you have to remove the check here, is it possible to add a check somewhere else?
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 compared the analysis increment of beta_s0=0 and beta_s0=0.15. Could you see the example of analysis increment shown in PR-543_DBZ-DA-hybrid.pptx? In the test of beta_s0=0.15, the analysis increment is slightly smaller due to smaller ensemble B and substantial zero static B, but it looks reasonable and realistic.
And if you have to remove the check here, is it possible to add a check somewhere else?
I added the warning of zero static B for reflectivity instead of fully removing this part. Does that solve your concern?
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 comparing the analysis and adding the warning message - this information is important. I am okay with the change and will approve the PR now.
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 static B is not extended for reflectivity DA for now, so the static B for reflectivity is regarded as zero in the setting of hybrid EnVar. This is not necssarily the best for reflectivity DA, but zero static-B (ignoring the impact of reflectivity DA in the static-B part) itself is not so strange setting. In addition, the influence of zero static-B is not so large
in case that the weight of ensemble B is much larger than that of static B.
@shoyokota What about the case the ensemble weight is 0.5 (3DRTMA used this setting in some situations) or smaller?
A general question, is it possible to use a different ensemble weight for different variables?
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.
@guoqing-noaa The smaller ensemble weight, the smaller impact of reflectivity DA (the closer to the situation of zero-B for the variables not included in static-B). The weight is fixed in the current code, but I think it is possible to make the weight changeable.
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, @shoyokota
It will be good if we can still be able to use 100% EnVar for Ref and similar variables while doing hybrid EnVar for other variables in one step.
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.
@guoqing-noaa In the current GSI code, the implementation is not so simple. If we apply pure EnVar only except for variables in static-B (sf,vp,t,ps), cross-variable covariance between (sf,vp,t,ps) and the other variables becomes larger, which affects not only the radar reflectivity DA but also the conventional DA. It is out of the objective of this PR and not tested at all. Even currently, humidity is not included in the variables in static-B, so I believe zero static-B also for the other variables including hydrometeors is currently the most straightforward way. It is very interesting to introduce the changeable weight for different variables, but it is better to think about it in another PR after evaluated carefully.
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.
@guoqing-noaa In the current GSI code, the implementation is not so simple. If we apply pure EnVar only except for variables in static-B (sf,vp,t,ps), cross-variable covariance between (sf,vp,t,ps) and the other variables becomes larger, which affects not only the radar reflectivity DA but also the conventional DA. It is out of the objective of this PR and not tested at all. Even currently, humidity is not included in the variables in static-B, so I believe zero static-B also for the other variables including hydrometeors is currently the most straightforward way. It is very interesting to introduce the changeable weight for different variables, but it is better to think about it in another PR after evaluated carefully.
I agree. Thanks, @shoyokota
As per the 4/10/2023 email sent to the GSI subversion email list, PRs must be merged within 6 weeks of creation. PRs not merged within the 6 week window will be returned to the originating developer (see GSI Wiki). This PR was opened before this announcement . The due date for this PR is set to 5/22/2023, six weeks after 4/10/2023. |
7407ff0
to
d66e1d2
Compare
@daviddowellNOAA Do you get a chance to review this ticket? Before May 22, we have to finish the review process. |
Reminder: This review will be closed and returned to the developer if not merged before 5/23/2023 |
@hu5970 Do have time to review this PR? |
@shoyokota The code changes look good to me. I have a question about the option if_use_w_vr. If I understand correctly, you intend to turn it off when assimilating radar reflectivity only, right? However, if the assimilated observations do not include radial velocity, the changed codes setuprw.f90 intrw.f90, and stprw.f90 won't be used in the assimilation. Could you please clarify the changes? Thank you. |
@Wangy1111 Thank you for reviewing it.
I intend to use "if_use_w_vr=false" only when assimilating radar reflectivity and radial velocity together. This option changes the observation operator of "radial velocity" not to use vertical wind (w) even when w is in the state vector (for reflectivity DA). In the case that radial velocity is not assimilated, this option is not required to be activated. |
@shoyokota Thank you for the clarification. I will approve the code now. |
@shoyokota Do you have a chance to look at Yongming's question? Hopefully, we can finish review processing before 5/22. |
@ShunLiu-NOAA I answered Yongming's question and he approved this PR yesterday. Is there something I additionally should do? |
@shoyokota Looks good to me. I will make a regression test on WCOSS2 before merging codes. |
@shoyokota could you please sync your fork with develop so that we can run regression test? |
d66e1d2
to
688c3d2
Compare
@ShunLiu-NOAA I merged the current develop branch to my branch with "git rebase". |
… conventional data simultaneously without side effects in EnVar
688c3d2
to
f60263d
Compare
@ShunLiu-NOAA I updated my branch with "git rebase" again. |
@@ -24,7 +24,7 @@ module gsimod | |||
|
|||
use obsmod, only: doradaroneob,oneoblat,oneoblon,oneobheight,oneobvalue,oneobddiff,oneobradid,& | |||
radar_no_thinning,ens_hx_dbz_cut,static_gsi_nopcp_dbz,rmesh_dbz,& | |||
rmesh_vr,zmesh_dbz,zmesh_vr,if_vterminal, if_model_dbz,if_vrobs_raw,& | |||
rmesh_vr,zmesh_dbz,zmesh_vr,if_vterminal, if_model_dbz,if_vrobs_raw,if_use_w_vr,& |
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.
Sorry, a picky comment here about the naming of "if_use_w_vr".
Without any further context, I would assume "if_use_w_vr" means using "w" and "vr".
suggest changing to a more verbose form, such as "if_use_w_in_vr", "if_use_w_for_vr", or "if_3D_wind_for_vr", etc. Thanks!
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.
@guoqing-noaa Good suggestion. @shoyokota Since it is easy changes, you may consider to include the variable name changes in your next PR. Because the PR already exceed the 6 weeks limit and I just finished a set of regression test, I'd like to merge this PR into "develop" today.
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.
@guoqing-noaa @ShunLiu-NOAA Thank you for your comments. Yes, it is easy. I will think about changing this name if I create another PR related to the observation operator of radar radial wind.
Test project /lfs/h2/emc/lam/noscrub/emc.lam/Shun.Liu/gsi_regression/GSI/build 100% tests passed, 0 tests failed out of 9 |
This PR removes forcing pure EnVar and adding an option (if_use_w_vr) to assimilate radar reflectivity and conventional data simultaneously without side effects in EnVar (#540). Regression tests for global 3dvar/4denvar/4dvar are not completed yet, but for other tests, issues are not found except for "failed the scalability test" and "exceeded maximum allowable hardware memory limit" on Orion.
Fixes #540