-
Notifications
You must be signed in to change notification settings - Fork 153
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#341. Update the use of MW radiances for GFS v16.3 implementation #350
Conversation
@MichaelLueken-NOAA I suggest the following people for our internal review 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.
A few minor questions and changes.
I flagged a bunch of changes from Scott in TaTb which I think can be removed.
@@ -81,6 +81,10 @@ export imp_physics=${imp_physics:-99} | |||
lupp=${lupp:-".true."} | |||
cnvw_option=${cnvw_option:-".false."} | |||
|
|||
# Observation usage options | |||
cao_check=${cao_check:-".false."} | |||
ta2tb=${ta2tb:-".false."} |
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 know the intention is to not make any chnages to the results with this pull request, but the CAO check is something that really makes sense and I think it would be better if this defaulted to 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.
@ADCollard First, thanks for reviewing this PR.
Yes, I will make cao_check to true as default when we do the high-resolution parallel experiment. If the results look good, I will make cao_check=true as default in the GSI master and also in the package we hand over to NCO.
src/gsi/cplr_gfs_ensmod.f90
Outdated
@@ -1015,7 +1057,8 @@ subroutine parallel_read_gfsnc_state_(en_full,m_cvars2d,m_cvars3d,nlon,nlat,nsig | |||
enddo | |||
deallocate(rwork3d1) | |||
|
|||
if (k3u==0.or.k3v==0.or.k3t==0.or.k3q==0.or.k3cw==0.or.k3oz==0) & | |||
! if (k3u==0.or.k3v==0.or.k3t==0.or.k3q==0.or.k3cw==0.or.k3oz==0) & !orig |
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.
If these changes are permanent, remove comments.
What about k3ql, k3qi, k3qr, k3qs, k3qg?
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 current one actually works for operational configuration and for the configuration with precipitation turned on without checking k3ql, k3qi, k3qr, k3qs, and k3qg. So, this is not a strict check.
I will see if I can find a way to make it a strict check.
! enddo | ||
! errf(1:ich544)=zero | ||
! varinv(1:ich544)=zero | ||
! do i=1,ich544 |
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.
Remove commented code
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.
@ADCollard These lines are for future testing. So, they are commented out for now.
@@ -1047,7 +1052,7 @@ subroutine setuprad(obsLL,odiagLL,lunin,mype,aivals,stats,nchanl,nreal,nobs,& | |||
endif | |||
endif | |||
! Screening for cold-air outbreak area (only applied to MW for 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.
Does this mean the cao check is only performed when precip is turned on?
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.
@ADCollard Yes, the detection of CAO requires to use all of the five hydrometeors.
@MichaelLueken-NOAA
They are in feature/v16x_allsky under fix directory. |
@emilyhcliu You don't need to do anything. Once the work in this PR has been merged to the master, I will need to manually merge the changes in feature/v16x_allsky into rev2. I need to do this whether the updated fix hash is in the PR or not, so all I need is the fix branch to merge to rev2 (which you have provided) and that is it. |
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.
Hi @emilyhcliu, I have completed my initial review of your changes. The first issue is that there are four commit messages. To reduce this to a single commit message, please do the following in your feature/v16x_allsky branch in your fork:
- git reset --soft HEAD~1
- git reset --soft HEAD~1
- git reset --soft HEAD~1
- git commit --amend
In the commit message window, please add:
GitHub Issue NOAA-EMC/GSI#341
to the commit message:
GitHub Issue NOAA_EMC/GSI#341. This update includes the following:
(1) Add the capability of assimilating precipitation-affected radiances from AMSU-A and ATMS
(2) Enable the all-sky radiance assimilation for GMI
(3) Add the switch to use brightness temperature (SDR) instead of antenna temperature (TDR) for AMSU-A, ATMS and MHS
Once the commit messages have been corrected, please go through and address the concerns I noted (mostly, removal of commented out lines of code that if they won't be used in the future). Once the changes have been made, please use:
- git commit --amend
- Save and close the commit message without changing the message.
- Push back to the repository using
git push origin feature/v16x_allsky --force
Please let me know if you have any questions, comments, or concerns.
src/gsi/cplr_gfs_ensmod.f90
Outdated
@@ -1015,7 +1057,8 @@ subroutine parallel_read_gfsnc_state_(en_full,m_cvars2d,m_cvars3d,nlon,nlat,nsig | |||
enddo | |||
deallocate(rwork3d1) | |||
|
|||
if (k3u==0.or.k3v==0.or.k3t==0.or.k3q==0.or.k3cw==0.or.k3oz==0) & | |||
! if (k3u==0.or.k3v==0.or.k3t==0.or.k3q==0.or.k3cw==0.or.k3oz==0) & !orig |
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.
If commented out line 1060 won't be used in the future, please remove it.
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.
Hi @emilyhcliu, I have completed my debug review of your changes in this PR. There are several unused variables that were flagged while compiling and I have noted them in this review. Please remove or comment out the unused variables (I say comment out because several of the unused variables are being flagged as such due to the code segments that they are used being commented out). Having said that, the debug executable ran through to completion, passing the debug tests. If you have any questions or concerns, please let me know.
src/gsi/general_read_gfsatm.f90
Outdated
real(r_kind),pointer,dimension(:,:) :: g_ps | ||
real(r_kind),pointer,dimension(:,:,:) :: g_vor,g_div,& | ||
g_q,g_oz,g_tv | ||
real(r_kind),pointer,dimension(:,:,:) :: g_ql,g_qi,g_qr,g_qs,g_qg,g_cf |
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.
While compiling in debug mode, g_cf, on line 2644, was noted as being unused. All code segments that use this variable are commented out. If these sections won't be used in the future, please remove g_cf. Otherwise, please comment out g_cf until the other commented out sections are uncommented.
src/gsi/write_incr.f90
Outdated
@@ -91,6 +91,9 @@ subroutine write_fv3_inc_ (grd,sp_a,filename,mype_out,gfs_bundle,ibin) | |||
use obsmod, only: ianldate | |||
use state_vectors, only: allocate_state, deallocate_state | |||
|
|||
use state_vectors, only: svars3d, ns3d |
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.
On line 94, ns3d was noted as unused. Please remove unused variable.
end if | ||
! test in the future | ||
! if (si_mean >= 20.0_r_kind) then |
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 si_mean is currently commented out, si_mean on line 2810 is now being flagged as unused. If possible, please move si_mean to the end of the passed variable list and comment it out until this section is brought back. Please note that this will also require some additional work in setuprad.f90.
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.
@MichaelLueken-NOAA I made si_mean an optional input.
1300c4d
to
33b8a04
Compare
@MichaelLueken-NOAA I fixed the commit comment as you instructed. I also addressed your comments and changed according. I had committed and push the changes back to the branch. Please let me know if you have any questions/comments. |
@emilyhcliu Your PR has too many commit messages. You will need to reduce the six commits down to one using:
If you have any questions, please let me know. |
33b8a04
to
60a3c49
Compare
Done! |
Hi @emilyhcliu. Thank you very much for addressing my concerns. There are just a few things I would like to note quickly:
After changes are made, please commit using: git commit --amend and push using: git push origin feature/v16x_allsky --force If you have any questions, please let me know. |
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.
Hi @emilyhcliu. Two more minor points in src/gsi/crtm_interface.f90. There are several commented out lines pertaining to CRTM v2.3.* (specifically, lines 2029 and 2083). Both of these lines appear to be identical. Additionally, they are identical to line 2087, which is not commented. Should these two commend out lines be removed?
src/gsi/crtm_interface.f90
Outdated
cloud_efr(k,ii)=max(5.001_r_kind, cloud_efr(k,ii)) | ||
endif | ||
end do | ||
!crtm2.3.x if (.not. regional .and. icfs==0 ) atmosphere(1)%cloud_fraction(k) = cf(kk2) |
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.
Should line 2083 be removed, since an uncommented out version of the line exists on line 2087?
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.
@MichaelLueken-NOAA You were right. I removed them.
src/gsi/crtm_interface.f90
Outdated
@@ -2000,7 +2026,7 @@ subroutine call_crtm(obstype,obstime,data_s,nchanl,nreal,ich, & | |||
if (ii==2 .and. atmosphere(1)%temperature(k)<t0c) & | |||
cloud_cont(k,2)=max(1.001_r_kind*1.0E-6_r_kind, cloud_cont(k,2)) | |||
end do | |||
!crtm2.3.x if (.not. regional .and. icfs==0 ) atmosphere(1)%cloud_fraction(k) = cf(kk2) | |||
!crtm2.3.x if (.not. regional .and. icfs==0 ) atmosphere(1)%cloud_fraction(k) = cf(kk2) |
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.
Should line 2029 be removed, since an uncommented out version of the line exists on line 2087?
60a3c49
to
73444cc
Compare
@MichaelLueken-NOAA I updated the codes as you suggested and pushed the back. |
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.
Hi @emilyhcliu. Thank you very much for making the requested changes. There is one last minor change. On line 759 of src/gsi/read_bufrtovs.f90, you replaced the first .ne. with /=, as requested. However, there is a second .ne. (sacv .ne. 1) that needs to also be replaced with /= (i.e., sacv /= 1). Once this change is made, I should be able to submit this work to the review committee once the current work has been merged to the authoritative repository.
After making the change, please use:
- git add src/gsi/read_bufrtovs.f90
- git commit --amend
- Save and close the commit message window without altering the current commit message
- git push origin feature/v16x_allsky --force
If you have any questions, please let me know.
src/gsi/read_bufrtovs.f90
Outdated
else ! EARS / DB | ||
call ufbrep(lnbufr,data1b8,1,nchanl,iret,'TMBRST') | ||
!if ( amsua .or. amsub .or. mhs .AND. sacv .ne. spc_coeff_versions)then | ||
if ( amsua .or. amsub .or. mhs .AND. spc_coeff_versions /= 1 .AND. sacv .ne. 1)then |
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 for replacing the first .ne. with /=. Please replace the second .ne. with /= on line 759.
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.
@MichaelLueken-NOAA Thanks you for your thorough review of my PR. Your request has been addressed.
This update includes the following: (1) Add the capability of assimilating precipitation-affected radiances from AMSU-A and ATMS (2) Enable the all-sky radiance assimilation for GMI (3) Add the switch to use brightness temperature (SDR) instead of antenna temperature (TDR) for AMSU-A, ATMS and MHS
73444cc
to
3c361da
Compare
The due date for the review committee has passed without feedback. I will now give final approval to these changes and merge them to the authoritative repository. |
The fix files in fix/feature/v16x_allsky were merged to fix/rev2 and pushed to the authoritative repository at 87fe77d. |
GitHub Issue NOAA-EMC#341. Update the use of MW radiances for GFS v16.3 implementation
GitHub Issue NOAA-EMC#341. Update the use of MW radiances for GFS v16.3 implementation
The following changes will be added to GSI Master for v16.3 Implementation:
Source code update:
Fix files update:
Script update:
Working Branch: feature/v16x_allsky
List of changes for Ta2Tb work:
Set ta2tb = .true. will trigger the use of Tb for AMSU-A, ATMS, and MHS
Default ta2tb is set to false. This ensures that the code update will not change GSI result from the master
List of changes for assimilating precipitation-affected ATMS and AMSU-A:
These code changes will not affect GSI result from master unless the default global_anavinfo.l127.txt is replaced with global_anavinfo_alhydro.l127.txt
The current GSI master (and operational GDAS) is using CRTM2.3.0 for radiance assimilation
The use of precipitation-affected MW data should combined with the use of CRTM 2.4.0 for the following two reasons:
Note: If we decide not to include precipitation-affected MW radiance in our next implementation, then we should not use CRTM2.4.0 (should stay with CRTM 2.3.0) due to the following two reasons:
List of changes for the assimilation of GMI under all-sky conditions:
These code update will not change GSI result from the master since GMI channel use flags are set to -1 (monitoring)
To activate GMI for assimilation, modifiy the use flags as the following:
List of changes for exglobal_atmos_analysis.sh: