-
Notifications
You must be signed in to change notification settings - Fork 146
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#337. Add capability to assimilate PM2.5 and VIIRS AOD for RRFS-CMAQ. #365
Conversation
Hi @hongli-wang, there are three commit messages associated with this work. It should only be one. To reduce the number of commit messages from three to one, please use the following commands in your fork's feature/RRFS-CMAQ branch:
The I will go ahead and close PR #334 at this time. If you have any questions, please let me know. |
12f5982
to
8bbfc17
Compare
@MichaelLueken-NOAA Thanks. I had followed the steps and pushed back to feature/RRFS-CMAQ. |
@hongli-wang Thanks, the commit message looks good. |
@@ -749,14 +759,98 @@ subroutine read_fv3_netcdf_guess(fv3filenamegin) | |||
real(r_kind),dimension(:,:,:),pointer::ges_qg=>NULL() | |||
real(r_kind),dimension(:,:,:),pointer::ges_qnr=>NULL() | |||
real(r_kind),dimension(:,:,:),pointer::ges_w=>NULL() | |||
|
|||
|
|||
real(r_kind),dimension(:,:,:),pointer::ges_aalj=>NULL() |
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 block of chem. Var declaration could be put into a include file? Using include files still conforms to GSI coding standard? @MichaelLueken-NOAA
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 @TingLei-NOAA. What @hongli-wang has done here should be fine. There are no coding standards requiring the use of include files for blocks of variable declarations. Most of the GSI doesn't use include files. So, it would probably be best to leave the block of chem variable declarations as they currently are.
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 , accepted. Thanks.
Then @hongli-wang , you can skip this comment.
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 all.
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 comments/questions, but mostly looks good I think. A lot of the if statements should be indented properly for readability.
src/gsi/amassaeromod.f90
Outdated
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it),'amassj',ges_amassj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it),'amassk',ges_amassk,istatus );ier=ier+istatus | ||
|
||
if (ier/=0) 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.
Redundant extra if statement
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.
agree. will fix. thanks.
src/gsi/amassaeromod.f90
Outdated
do i=1,lat2 | ||
sv_rank3(i,j,k)=cv_amassj(i,j,k)*ges_aero(i,j,k)/ges_amassj(i,j,k) | ||
if(ges_aero(i,j,k)/ges_amassj(i,j,k).ge.1.0)then | ||
!print*,"amas2aero_tl:wg.ge.1 ",trim(aerosols(ic)),ges_aero(i,j,k),ges_amassj(i,j,k) |
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 if doesn't do anything if this is commented out.
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 if doesn't do anything if this is commented out.
Agree. Deleted the if statement.
src/gsi/amassaeromod.f90
Outdated
call gsi_bundlegetpointer (rval,trim(aerosols(ic)),sv_rank3,istatus);ier=ier+istatus | ||
if (ier/=0) cycle | ||
|
||
print*,"amass2aero_ad: sum= ",ic,trim(aerosols(ic)),sum(sv_rank3),sum(cv_amassi),sum(cv_amassj),sum(cv_amassk) |
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.
Isn't this going to print a lot of stuff to stdout (2 lines per processor per aerosol species)? Consider removing/commenting out.
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.
commented out. Thanks.
src/gsi/chemmod.f90
Outdated
@@ -164,8 +439,14 @@ function obs2model_anowbufr_pm() | |||
obs2model_anowbufr_pm=cmaq_pm | |||
elseif (wrf_mass_regional) then | |||
obs2model_anowbufr_pm=wrf_chem_pm | |||
! elseif (fv3_cmaq_regional) 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.
is this intended to be commented out?
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.
is this intended to be commented out?
yes. removed.
src/gsi/control2state.f90
Outdated
id=getindex(cvars3d,gases(ic)) | ||
if (id>0) then | ||
call gsi_bundlegetpointer (sval(jj),gases(ic),sv_rank3,istatus) | ||
call gsi_bundlegetvar (wbundle, gases(ic),sv_rank3,istatus) | ||
!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.
Can we remove this section if it is commented out?
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.
Agree. Deleted.
src/gsi/obsmod.F90
Outdated
@@ -157,6 +157,9 @@ module obsmod | |||
! GSI namelist level. | |||
! 2020-09-15 Wu - add option tcp_posmatch to mitigate possibility of erroneous TC initialization | |||
! 2020-09-19 CAPS(J. Park) - add 'vad_near_analtime' flag to assimilate newvad obs around analysis time only | |||
! 2021-11-16 Zhao - add option l_obsprvdiag (if true) to trigger the output of |
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.
Same as before, why are these changes included here?
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.
copied from emc master (Apr 5) which is a little advanced than mine ( March 8). Just realized this today.
|
||
! new viirs table | ||
character (len=69) :: vaodgstr1 = & | ||
'SAID CLATH CLONH YEAR MNTH DAYS HOUR MINU SOZA SOLAZI RSST AOTQ RETRQ' |
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 break the global AOD regression test?
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.
Never mind, I see the below boolean
src/gsi/read_aerosol.f90
Outdated
rsat = hdrvaodg(1); ksatid=rsat | ||
|
||
if ( jsatid == 'NPP' .or. jsatid == 'npp' ) kidsat = 225 | ||
!if ( jsatid == 'NPP' .or. jsatid == 'npp' ) kidsat = 225 |
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 this if it is the wrong 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.
removed.
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 correct sat id for NPP should be 224.
src/gsi/read_guess.F90
Outdated
@@ -171,6 +173,9 @@ subroutine read_guess(iyear,month,idd,mype) | |||
else if (fv3_regional ) then | |||
call bg_fv3regfilenameg%init | |||
call read_fv3_netcdf_guess(bg_fv3regfilenameg) | |||
!if (fv3_cmaq_regional)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.
Is this not needed because it will read it in the above line?
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.
Previously I had a separated subroutine to read in the aero variables. But now, I keep this capability in the read_fv3_netcdf_guess. There is not such a subroutine anymore. I will remove the lines.
src/gsi/write_all.F90
Outdated
@@ -121,6 +122,9 @@ subroutine write_all(increment) | |||
if (regional) then | |||
if (fv3_regional) then | |||
call wrfv3_netcdf(bg_fv3regfilenameg) | |||
!if (fv3_cmaq_regional) 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.
Same question as in the read, if this isn't needed, remove, don't comment out.
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.
will remove.
src/gsi/gsi_rfv3io_mod.f90
Outdated
@@ -1018,6 +1180,109 @@ subroutine read_fv3_netcdf_guess(fv3filenamegin) | |||
call GSI_BundleGetPointer ( GSI_MetGuess_Bundle(it), 'w' , ges_w ,istatus );ier=ier+istatus | |||
end if | |||
|
|||
if (laeroana_fv3cmaq) 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.
This block ( line1183-1284, laroana_fv3cmaq=.true.) and other similar blocks containing almost 100 lines can be put into one internal subroutine, and, hence, keep the main subroutine much cleaner
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. will consider.
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.
@hongli-wang I notice this kind of blocks containing about 100 lines of calls for GSI_BundleGetPointer sill exist. Would it be much more readable as suggested using a nested contain for a subroutine? To do this change only need a few vi actions . For this part, I made an example (successfully complied) in /scratch2/NCEPDEV/fv3-cam/Ting.Lei/dr-hongli/dr-pr-cmaq/GSI/src/gsi/gsi_rfv3io_mod.f90. A snipping shot is as
Searching "example" . Maybe, a better way is to create a module outside of gsi_rfv3io_mod.f90 to declare and do this kind of actions for this large patch of airosal variables. But current way is the least effort needed.
src/gsi/gsi_rfv3io_mod.f90
Outdated
endif | ||
|
||
if( fv3sar_bg_opt == 0) then | ||
call GSI_BundleGetPointer ( gsibundle_fv3lam_dynvar_nouv, 'delp' ,ges_delp ,istatus );ier=ier+istatus | ||
if (laeroana_fv3cmaq) 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.
The same suggestions , this block when laeroana_fv3camq=.true.), could be put into a internal subroutine.
after that, this block would be like
^^
if (laeroana_fv3cmq) then
call aeroana_getpoint
endif
VV
That will look cleaner.
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. will consider.
src/gsi/gsi_rfv3io_mod.f90
Outdated
vartem=trim(name_chemvars3d(i)) | ||
if (ifindstrloc(aeronames_cmaq_fv3,trim(vartem)) > 0) then | ||
jtracer=jtracer+1 | ||
fv3lam_io_tracerchemvars3d_nouv(jtracer)=trim(vartem) |
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 question: the chem. vars are also in the tracer file. If the IO of chem vars are delt along with other tracer files,
namely, combining fv3lam_io_tracerchemvars3d_nouv ( and corressponding IO bundle ) and fv3lam_io_tracervars3d_nouv,this will cause a more efficient/scalable parallel IO.
Will this add any significant complicities to the coding?
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 is a good question/suggestion. I thought about this when I updated my code. If you think the combination will improve the efficiency of parallel IO, I may reconsider it. 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.
The current parallelization is done over number of variable number multiplied by the number of vertical levels. Assume, vertical levels are 60 levels, in the original tracers, there are two tracer variables , and , 7 chem. variables. In the current scheme, first, 2X60 fields would be distributed to various MPI processes and their IO (and conversion between native and analysis grids) would be done in parallel; second, after the first step, the similar would be done chem. variable, now, it is 7X60 fields would be deal with in parallel. If they are done together, there would be only one step: (2+7)X60 fields would be dealt with in parallel. In this case, if the number of mpi process is more than 120, then, in the first step of the current scheme, some MPI process would do nothing and namely the scalibility is not so well. Hope my explanation will help!
8bbfc17
to
ae51201
Compare
cbed89b
to
840cb19
Compare
@CoryMartin-NOAA @TingLei-NOAA At your earliest convenience, please review that the modifications made by @hongli-wang address your concerns. If you are okay with the changes, please approve the PR. Otherwise, please note the issues that you have so that @hongli-wang may address them. Thank you very much for your time. |
I will review by COB tomorrow. |
Hi, Mike,
I am starting to review it. But maybe, I will finish my reviewing till next
week.
Regards,
Ting
…______________________________
Ting Lei
IMSG at NOAA/NWS/NCEP/EMC
5830 University Research Ct., Cubicle 2765
College Park, MD 20740
***@***.***
301-683-3624
On Thu, May 19, 2022 at 10:53 AM Cory Martin ***@***.***> wrote:
I will review by COB tomorrow.
—
Reply to this email directly, view it on GitHub
<#365 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/APEFS7EC7VBPHOKYG7PLWK3VKZIXVANCNFSM5T5FCZ2A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
c54a8c4
to
0afecd7
Compare
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 to the developer for taking care of all the questions/suggestions.
This PR contains quite a lot of development in GSI, which have been implemented in GSI's style, which is not only efficient but also make the new codes have a good readability in terms of the coding itself.
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.
Echoing Ting's comments, thanks for all of your hard work on this!
Hi @hongli-wang - Last Friday, I force-pushed an update to NOAA-EMC/GSI's develop branch. When I did this, your PR was force updated as well (since the branch associated with this PR was created directly from the develop branch). This has led to several conflicts and 10, rather than 1, commit message. To correct this:
This should correct the conflicts and bring the number of commit messages back to 1. If you have any questions or encounter any issues, please let me know. |
@MichaelLueken-NOAA I will take care the conflicts evening. Since most of files are unrelated to my changes, I may check out gsi develop branch and copy the files there to resolve the conflicts. I will let you know when I resolve the conflicts. Thanks! |
0afecd7
to
dbd3f2c
Compare
@MichaelLueken-NOAA I have pushed the changes back by fellowing your instructions. Please double check and let me know if you see any issues. Thanks again! |
dbd3f2c
to
466ad51
Compare
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 have completed my initial review of your changes. Please ensure that all if statements and do loops have at least two-space indentation. Please ensure that all real and integer declarations need either a length (i_kind) or precision (r_kind) and all real values need a precision (_r_kind). Please ensure that modified lines don't exceed the 132 character limit. Please ensure that all .ge., .lt., and .eq. are replaced with >=, <, and ==, respectively. Please ensure that implicit none
is included for all subroutines.
I haven't run the regression tests yet. I also haven't tried compiling the changes using GNU compilers. I'll return with any issues that might show while attempting to run the regression (standard and debug) tests and compiling with GNU.
src/gsi/amassaeromod.f90
Outdated
|
||
|
||
! Get background amassi,amassj,amassk | ||
ier = 0.0 |
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.
ier is an integer. Please use 0 rather than real 0.0
src/gsi/amassaeromod.f90
Outdated
real(r_kind),pointer,dimension(:,:,:) :: ges_amassk=>NULL() | ||
|
||
|
||
ier = 0.0 |
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.
ier is an integer. Please replace 0.0 with 0.
src/gsi/amassaeromod.f90
Outdated
|
||
! ad of partitioning amass into aerosol speccies | ||
do ic=1,naerosols | ||
ier = 0.0 |
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.
ier is an integer. Please replace 0.0 with 0.
src/gsi/amassaeromod.f90
Outdated
do ic=1,naerosols | ||
call GSI_BundleGetPointer (GSI_ChemGuess_Bundle(it),trim(aerosols(ic)),ges_aero,istatus ) | ||
call gsi_bundlegetpointer (sval,trim(aerosols(ic)),sv_rank3,istatus) | ||
if (istatus/=0) cycle | ||
sv_rank3=zero | ||
if(imodes_cmaq_fv3(ic).eq.1)then | ||
do k=1,nsig | ||
do j=1,lon2 | ||
do i=1,lat2 | ||
sv_rank3(i,j,k)=cv_amassi(i,j,k)*ges_aero(i,j,k)/ges_amassi(i,j,k) | ||
end do | ||
end do | ||
end do | ||
elseif(imodes_cmaq_fv3(ic).eq.2)then | ||
do k=1,nsig | ||
do j=1,lon2 | ||
do i=1,lat2 | ||
sv_rank3(i,j,k)=cv_amassj(i,j,k)*ges_aero(i,j,k)/ges_amassj(i,j,k) | ||
end do | ||
end do | ||
end do | ||
else | ||
do k=1,nsig | ||
do j=1,lon2 | ||
do i=1,lat2 | ||
sv_rank3(i,j,k)=cv_amassk(i,j,k)*ges_aero(i,j,k)/ges_amassk(i,j,k) | ||
end do | ||
end do | ||
end do | ||
|
||
end if | ||
end do | ||
|
||
return |
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.
Please align line 78 with line 75. Please ensure all if statements and do loops have at least two-space indentation. Please replace .eq. with == on lines 83 and 91.
src/gsi/amassaeromod.f90
Outdated
do ic=1,naerosols | ||
ier = 0.0 | ||
call GSI_BundleGetPointer (GSI_ChemGuess_Bundle(it),trim(aerosols(ic)),ges_aero,istatus );ier=ier+istatus | ||
call gsi_bundlegetpointer (rval,trim(aerosols(ic)),sv_rank3,istatus);ier=ier+istatus | ||
if (ier/=0) cycle | ||
|
||
if(imodes_cmaq_fv3(ic).eq.1)then | ||
do k=1,nsig | ||
do j=1,lon2 | ||
do i=1,lat2 | ||
cv_amassi(i,j,k)=cv_amassi(i,j,k)+sv_rank3(i,j,k)*ges_aero(i,j,k)/ges_amassi(i,j,k) | ||
end do | ||
end do | ||
end do | ||
elseif(imodes_cmaq_fv3(ic).eq.2)then | ||
do k=1,nsig | ||
do j=1,lon2 | ||
do i=1,lat2 | ||
cv_amassj(i,j,k)=cv_amassj(i,j,k)+sv_rank3(i,j,k)*ges_aero(i,j,k)/ges_amassj(i,j,k) | ||
end do | ||
end do | ||
end do | ||
else | ||
do k=1,nsig | ||
do j=1,lon2 | ||
do i=1,lat2 | ||
cv_amassk(i,j,k)=cv_amassk(i,j,k)+sv_rank3(i,j,k)*ges_aero(i,j,k)/ges_amassk(i,j,k) | ||
end do | ||
end do | ||
end do | ||
|
||
end if | ||
end do |
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.
Please align line 154 with line 151. Please ensure all if statements and do loops have at least two-space indentation. Please replace .eq. with == on lines 160 and 168.
src/gsi/gsi_rfv3io_mod.f90
Outdated
! real(r_kind),dimension(:,:,:),pointer::ges_pm25at=>NULL() | ||
! real(r_kind),dimension(:,:,:),pointer::ges_pm25ac=>NULL() | ||
! real(r_kind),dimension(:,:,:),pointer::ges_pm25co=>NULL() | ||
! real(r_kind),dimension(:,:,:),pointer::ges_pm2_5=>NULL() | ||
|
||
! real(r_kind),dimension(:,:,:),pointer::ges_amassi=>NULL() | ||
! real(r_kind),dimension(:,:,:),pointer::ges_amassj=>NULL() | ||
! real(r_kind),dimension(:,:,:),pointer::ges_amassk=>NULL() |
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.
Please remove commented out lines if you won't be using them in the future.
src/gsi/gsi_rfv3io_mod.f90
Outdated
if (laeroana_fv3cmaq)then | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aalj',ges_aalj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'acaj',ges_acaj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'acli',ges_acli,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aclj',ges_aclj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aclk',ges_aclk,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'acors',ges_acors,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aeci',ges_aeci,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aecj',ges_aecj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'afej',ges_afej,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aivpo1j',ges_aivpo1j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'akj',ges_akj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'alvoo1i',ges_alvoo1i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'alvoo2i',ges_alvoo2i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'alvpo1i',ges_alvpo1i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'alvpo1j',ges_alvpo1j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'amgj',ges_amgj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'amnj',ges_amnj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'anai',ges_anai,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'anaj',ges_anaj,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'anh4i',ges_anh4i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'anh4j',ges_anh4j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'anh4k',ges_anh4k,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'ano3i',ges_ano3i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'ano3j',ges_ano3j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'ano3k',ges_ano3k,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aothri',ges_aothri,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aothrj',ges_aothrj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aseacat',ges_aseacat,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asij',ges_asij,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aso4i',ges_aso4i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aso4j',ges_aso4j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aso4k',ges_aso4k,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asoil',ges_asoil,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvoo1i',ges_asvoo1i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvoo2i',ges_asvoo2i,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvpo1i',ges_asvpo1i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvpo1j',ges_asvpo1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvpo2i',ges_asvpo2i,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvpo2j',ges_asvpo2j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvpo3j',ges_asvpo3j,istatus );ier=ier+istatus | ||
|
||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'atij',ges_atij,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'atol1j',ges_atol1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'atol2j',ges_atol2j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'atol3j',ges_atol3j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'abnz1j',ges_abnz1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'abnz2j',ges_abnz2j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'abnz3j',ges_abnz3j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aiso1j',ges_aiso1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aiso2j',ges_aiso2j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aiso3j',ges_aiso3j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'atrp1j',ges_atrp1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'atrp2j',ges_atrp2j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asqtj' ,ges_asqtj,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aalk1j',ges_aalk1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aalk2j',ges_aalk2j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'apah1j',ges_apah1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'apah2j',ges_apah2j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'apah3j',ges_apah3j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aorgcj',ges_aorgcj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aolgbj',ges_aolgbj,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'aolgaj',ges_aolgaj,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'alvoo1j',ges_alvoo1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'alvoo2j',ges_alvoo2j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvoo1j',ges_asvoo1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvoo2j',ges_asvoo2j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'asvoo3j',ges_asvoo3j,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'apcsoj',ges_apcsoj,istatus );ier=ier+istatus | ||
|
||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'axyl1j',ges_axyl1j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'axyl2j',ges_axyl2j,istatus );ier=ier+istatus | ||
call GSI_BundleGetPointer ( GSI_ChemGuess_Bundle(it), 'axyl3j',ges_axyl3j,istatus );ier=ier+istatus | ||
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.
Please ensure at least two-space indentation for all if statements.
src/gsi/gsi_rfv3io_mod.f90
Outdated
if (laeroana_fv3cmaq) then | ||
call gsi_copy_bundle(GSI_ChemGuess_Bundle(it),gsibundle_fv3lam_tracerchem_nouv) | ||
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.
Please ensure at least two-space indentation for all if statements.
src/gsi/gsi_rfv3io_mod.f90
Outdated
if (laeroana_fv3cmaq) then | ||
call gsi_fv3ncdf_write(grd_fv3lam_tracerchem_ionouv,gsibundle_fv3lam_tracerchem_nouv, & | ||
add_saved,fv3filenamegin%tracers,fv3filenamegin) | ||
endif |
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.
Please ensure at least two-space indentation for if statements.
src/gsi/gsi_rfv3io_mod.f90
Outdated
if (laeroana_fv3cmaq) then | ||
call gsi_fv3ncdf_write_v1(grd_fv3lam_tracerchem_ionouv,gsibundle_fv3lam_tracerchem_nouv,& | ||
add_saved,fv3filenamegin%tracers,fv3filenamegin) | ||
endif |
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.
Please ensure at least two-space indentation for if statements.
60e29a1
to
bbd4721
Compare
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 very much for making updates following my review, @hongli-wang! There are a few more issues to iron out, then this work should be ready for the review committee.
src/gsi/crtm_interface.f90
Outdated
@@ -555,10 +559,16 @@ subroutine init_crtm(init_pass,mype_diaghdr,mype,nchanl,nreal,isis,obstype,radmo | |||
if (radmod%laerosol_fwd) then | |||
if(.not.allocated(aero)) allocate(aero(nsig,n_actual_aerosols)) | |||
if(.not.allocated(aero_conc)) allocate(aero_conc(msig,n_actual_aerosols),auxrh(msig)) | |||
!wang if(.not.allocated(aero_wc)) allocate(aero_wc(msig,n_actual_aerosols)) |
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.
Please remove name from comment. If code won't be used in the future, please remove the line.
src/gsi/set_crtm_aerosolmod.f90
Outdated
aerosol(i)%type = SULFATE_CMAQ_AEROSOL | ||
case ('aclj','aclk','anai','anaj','aseacat') | ||
aerosol(i)%type = SEASALT_AEROSOL | ||
case ('aivpo1j','alvpo1i','alvpo1j','aothri','aothrj','asvpo1i','asvpo1j','asvpo2i','asvpo2j','asvpo3j','atol1j','axyl1j','axyl2j','axyl3j') |
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 line exceeds the 132 character limit. Please break this line down so that it doesn't exceed 132 characters.
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.
Please break up this line that exceeds the 132 character limit.
src/gsi/setupaod.f90
Outdated
@@ -373,13 +373,36 @@ subroutine setupaod(obsLL,odiagLL,lunin,mype,nchanl,nreal,nobs,& | |||
case( 23 ) ! over bright land surface, high quality | |||
tnoise = 0.0550472_r_kind+ 0.299558_r_kind*aod_obs | |||
end select | |||
if (fv3_cmaq_regional)then | |||
if (aod_obs(n_viirs_550nm) .lt. 0.4_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.
Please replace .lt. with <
src/gsi/setupaod.f90
Outdated
if (obstype == 'viirs_aod' .and. fv3_cmaq_regional)then | ||
do i = 1, nchanl | ||
if ( i /= n_viirs_550nm) cycle | ||
if (aod_obs(i) .lt. 0.4_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.
Please replace .lt. with <
src/gsi/setupaod.f90
Outdated
if (aod_obs(i) .lt. 0.4_r_kind) then | ||
aod_obs(i) = aod_obs(i) - ( 0.41_r_kind*aod_obs(i)-0.03_r_kind ) | ||
end if | ||
if (aod_obs(i) .ge. 0.4_r_kind .and. aod_obs(i) .lt. 0.9_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.
Please replace .ge. with >= and .lt. with <
src/gsi/gsi_rfv3io_mod.f90
Outdated
@@ -1566,7 +2001,12 @@ subroutine gsi_fv3ncdf_read(grd_ionouv,cstate_nouv,filenamein,fv3filenamegin) | |||
endif | |||
do ilevtot=kbgn,kend | |||
vgsiname=grd_ionouv%names(1,ilevtot) | |||
! write(6,*)' gsi_fv3ncdf_read_wang1: ',ilevtot,vgsiname |
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.
Please remove debug write statement.
src/gsi/gsi_rfv3io_mod.f90
Outdated
@@ -1591,8 +2031,10 @@ subroutine gsi_fv3ncdf_read(grd_ionouv,cstate_nouv,filenamein,fv3filenamegin) | |||
deallocate(uu2d_layout) | |||
enddo | |||
else | |||
!write(6,*)' gsi_fv3ncdf_read_wang2: ',ilevtot,varname |
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.
Please remove debug write.
src/gsi/gsi_rfv3io_mod.f90
Outdated
iret=nf90_inq_varid(gfile_loc,trim(adjustl(varname)),var_id) | ||
iret=nf90_get_var(gfile_loc,var_id,uu2d,start=startloc,count=countloc) | ||
!write(6,*)' gsi_fv3ncdf_read_wang3: ',ilevtot,var_id,sum(uu2d) |
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.
Please remove debug write.
bbd4721
to
86bfa71
Compare
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 @hongli-wang. The code compiles using Intel and GNU compilers without issues. The regression tests were run and all passed successfully. While compiling in debug, there were several unused variables in your changes that will need to be addressed (either removed or commented out) and have been noted in the review. Also, there was a subscript warning that will need to be addressed (the location and warning message are in this latest review). With respect to alignment in gsi_rfv3io_mod.f90 and stppm2_5, the if statements and do loops need to begin in the same column as the preceding line, then ensure two-space indentation within the if statements and do loops. In set_crtm_aerosolmod, please ensure that the line that exceeds 132 characters is broken up (line 131) and that all real values have a precision (_r_kind) associated with them.
src/gsi/set_crtm_aerosolmod.f90
Outdated
aerosol(i)%type = SULFATE_CMAQ_AEROSOL | ||
case ('aclj','aclk','anai','anaj','aseacat') | ||
aerosol(i)%type = SEASALT_AEROSOL | ||
case ('aivpo1j','alvpo1i','alvpo1j','aothri','aothrj','asvpo1i','asvpo1j','asvpo2i','asvpo2j','asvpo3j','atol1j','axyl1j','axyl2j','axyl3j') |
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.
Please break up this line that exceeds the 132 character limit.
0637009
to
f936ad3
Compare
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 very much, @hongli-wang! There is one more indentation issue in src/gsi_rfv3io_mod.90 that needs to be addressed, then I should be able to submit this work to the review committee.
src/gsi/gsi_rfv3io_mod.f90
Outdated
@@ -953,6 +1070,35 @@ subroutine read_fv3_netcdf_guess(fv3filenamegin) | |||
write(6,*)'fv3lam_io_dynmetvars2d_nouv is ',(trim(fv3lam_io_dynmetvars2d_nouv(i)),i=1,ntracerio3d) | |||
endif | |||
|
|||
if (laeroana_fv3cmaq) 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.
Please indent line 1073 by two spaces to align with line 1071 above.
src/gsi/gsi_rfv3io_mod.f90
Outdated
@@ -953,6 +1070,35 @@ subroutine read_fv3_netcdf_guess(fv3filenamegin) | |||
write(6,*)'fv3lam_io_dynmetvars2d_nouv is ',(trim(fv3lam_io_dynmetvars2d_nouv(i)),i=1,ntracerio3d) | |||
endif | |||
|
|||
if (laeroana_fv3cmaq) then | |||
jtracer = 0 |
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.
Please indent line 1074 by two spaces for proper if statement indentation.
src/gsi/gsi_rfv3io_mod.f90
Outdated
if (laeroana_fv3cmaq) then | ||
jtracer = 0 | ||
!name_chemvars3d chemguess from anavinfo | ||
do i=1,size(name_chemvars3d) |
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.
Please indent line 1076 by two spaces for proper if statement indentation.
src/gsi/gsi_rfv3io_mod.f90
Outdated
jtracer = 0 | ||
!name_chemvars3d chemguess from anavinfo | ||
do i=1,size(name_chemvars3d) | ||
vartem=trim(name_chemvars3d(i)) |
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.
Please indent line 1077 by two spaces for proper do loop indentation.
src/gsi/gsi_rfv3io_mod.f90
Outdated
!name_chemvars3d chemguess from anavinfo | ||
do i=1,size(name_chemvars3d) | ||
vartem=trim(name_chemvars3d(i)) | ||
if (ifindstrloc(aeronames_cmaq_fv3,trim(vartem)) > 0) 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.
Please indent line 1078 by two spaces for proper do loop indentation.
src/gsi/gsi_rfv3io_mod.f90
Outdated
fv3lam_io_tracerchemvars3d_nouv(jtracer+3)="pm25co" | ||
fv3lam_io_tracerchemvars3d_nouv(jtracer+4)="pm2_5" | ||
fv3lam_io_tracerchemvars3d_nouv(jtracer+5)="amassi" | ||
fv3lam_io_tracerchemvars3d_nouv(jtracer+6)="amassj" |
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.
Please indent line 1093 by two spaces for proper if statement indentation.
src/gsi/gsi_rfv3io_mod.f90
Outdated
fv3lam_io_tracerchemvars3d_nouv(jtracer+4)="pm2_5" | ||
fv3lam_io_tracerchemvars3d_nouv(jtracer+5)="amassi" | ||
fv3lam_io_tracerchemvars3d_nouv(jtracer+6)="amassj" | ||
fv3lam_io_tracerchemvars3d_nouv(jtracer+7)="amassk" |
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.
Please indent line 1074 by two spaces for proper if statement indentation.
src/gsi/gsi_rfv3io_mod.f90
Outdated
fv3lam_io_tracerchemvars3d_nouv(jtracer+6)="amassj" | ||
fv3lam_io_tracerchemvars3d_nouv(jtracer+7)="amassk" | ||
|
||
if (mype == 0) 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.
Please indent line 1096 by two spaces for proper if statement indentation.
src/gsi/gsi_rfv3io_mod.f90
Outdated
fv3lam_io_tracerchemvars3d_nouv(jtracer+7)="amassk" | ||
|
||
if (mype == 0) then | ||
write(6,*) ' fv3lam_io_tracerchemvars3d_nouv is',(trim(fv3lam_io_tracerchemvars3d_nouv(i)),i=1,ntracerchemio3d) |
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.
Please indent line 1097 by two spaces for proper if statement indentation.
src/gsi/gsi_rfv3io_mod.f90
Outdated
|
||
if (mype == 0) then | ||
write(6,*) ' fv3lam_io_tracerchemvars3d_nouv is',(trim(fv3lam_io_tracerchemvars3d_nouv(i)),i=1,ntracerchemio3d) | ||
endif |
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.
Please indent line 1098 by two spaces for proper if statement indentation.
f936ad3
to
54520ac
Compare
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 addressing my concerns, @hongli-wang. Once the current work has been merged to the authoritative repository tomorrow, I will submit this work to the review committee.
@MichaelLueken-NOAA Thank for your comments and let me know if you have other concerns. |
Hi @hongli-wang. An update to src/gsi/gsi_rfv3io_mod has been merged to the authoritative repository that causes a conflict with your update. Please rebase the authoritative develop branch to your feature/RRFS-CMAQ branch, then I should be able to submit your work to the review committee. |
54520ac
to
3f5374a
Compare
…RS AOD in RRFS-CMAQ.
3f5374a
to
38f938f
Compare
Hi @MichaelLueken-NOAA, I just rebased my code, it seems no conflict now. |
The due date for feedback from the review committee has past with no feedback, so I will now give final approval to these changes and merge this work to the authoritative develop branch. |
Add capability to assimilate PM2.5 and VIIRS AOD for RRFS-CMAQ.