Skip to content
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.

Sign up for GitHub

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

Jump to bottom

changes to allow USE_MGBF be able to be off #772

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

changes to allow USE_MGBF be able to be off #772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ab15cb
change to allow USE_MGBF be able to be off
TingLei-daprediction Jul 9, 2024
cf3a3a3
According to Russ' suggestion, using BUILD_MGBF on the top level to …
TingLei-daprediction Jul 12, 2024
f3ef08c
Merge branch 'develop' into feature/gsi_mgbf_turned_off
TingLei-daprediction Jul 15, 2024
4e85ebd
Merge branch 'develop' into feature/gsi_mgbf_turned_off
TingLei-daprediction Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ endif()
option(OPENMP "Enable OpenMP Threading" OFF)
option(ENABLE_MKL "Use MKL for LAPACK implementation (if available)" ON)
option(BUILD_GSDCLOUD "Build GSD Cloud Analysis Library" OFF)
option(BUILD_MGBF "Build MGBF Library" ON)
option(BUILD_MGBF "Build MGBF Library" OFF)
option(BUILD_GSI "Build GSI" ON)
option(BUILD_ENKF "Build EnKF" ON)
option(BUILD_REG_TESTING "Build the Regression Testing Suite" OFF)
Expand Down
5 changes: 4 additions & 1 deletion src/gsi/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ endif()
option(OPENMP "Enable OpenMP Threading" OFF)
option(ENABLE_MKL "Use MKL for LAPACK implementation (if available)" ON)
option(USE_GSDCLOUD "Use GSD Cloud Analysis library" OFF)
option(USE_MGBF "Use MGBF library" ON)
option(USE_MGBF "Use MGBF library" ${BUILD_MGBF})

set(GSI_VALID_MODES "GFS" "Regional")
set(GSI_MODE "GFS" CACHE STRING "Choose the GSI Application.")
Expand Down Expand Up @@ -132,6 +132,9 @@ endif()
if(USE_GSDCLOUD)
list(APPEND GSI_Fortran_defs RR_CLOUDANALYSIS)
endif()
if(USE_MGBF)
list(APPEND GSI_Fortran_defs USE_MGBF_def)
endif()

# Create a library of GSI C sources
add_library(gsi_c_obj OBJECT ${GSI_SRC_C})
Expand Down
28 changes: 27 additions & 1 deletion src/gsi/hybrid_ensemble_isotropic.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,10 @@ module hybrid_ensemble_isotropic
use string_utility, only: StrUpCase

! For MGBF
#ifdef USE_MGBF
use mg_intstate
use mg_timers
#endif /* End of USE_MGBF block */

implicit none

Expand Down Expand Up @@ -182,7 +184,9 @@ module hybrid_ensemble_isotropic
integer(r_kind) :: nval_loc_en

! For MGBF
#ifdef USE_MGBF_def
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USE_MGBF_def is the wrong variable. CMakeLists.txt sets USE_MGBF. USE_MGBF_def needs to be changed to USE_MGBF

The same comment applies to all occurrences of USE_MGBF_def in this source code file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use different names in cmake files and the fortran source codes
in src/gsi/CMakeLists.txt, there is :

if(USE_MGBF)
  list(APPEND GSI_Fortran_defs USE_MGBF_def)
endif()

That makes USE_MGBF_def to be used as the preprocessing option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set USE_MGBF to ON in CMakeLists.txt. This broke the build

[ 80%] Building Fortran object src/gsi/CMakeFiles/gsi_fortran_obj.dir/hybrid_ensemble_isotropic.F90.o
/lfs/h2/emc/da/noscrub/russ.treadon/git/gsi/test/src/gsi/hybrid_ensemble_isotropic.F90(188): error #6457: This derived type name has not been declared.   [MG_INTSTATE_TYPE]
  type (mg_intstate_type), allocatable, dimension(:) :: obj_mgbf
--------^
/lfs/h2/emc/da/noscrub/russ.treadon/git/gsi/test/src/gsi/hybrid_ensemble_isotropic.F90(4184): error #6158: The structure-name is invalid or is missing.   [OBJ_MGBF]
  real(r_kind)   ,intent(inout) :: g(grd_loc%nsig,obj_mgbf(ig)%nm,obj_mgbf(ig)%mm)
--------------------------------------------------^
/lfs/h2/emc/da/noscrub/russ.treadon/git/gsi/test/src/gsi/hybrid_ensemble_isotropic.F90(4184): error #6158: The structure-name is invalid or is missing.   [OBJ_MGBF]
  real(r_kind)   ,intent(inout) :: g(grd_loc%nsig,obj_mgbf(ig)%nm,obj_mgbf(ig)%mm)
------------------------------------------------------------------^
/lfs/h2/emc/da/noscrub/russ.treadon/git/gsi/test/src/gsi/hybrid_ensemble_isotropic.F90(1769): error #6404: This name does not have a type, and must have an explicit type.   [PRINT_CPU]
       if(l_mgbf_loc) call print_mg_timers("mgbf_timing_cpu.csv", print_cpu, mype)
------------------------------------------------------------------^
/lfs/h2/emc/da/noscrub/russ.treadon/git/gsi/test/src/gsi/hybrid_ensemble_isotropic.F90(3749): error #6404: This name does not have a type, and must have an explicit type.   [OBJ_MGBF]
     allocate(work_mgbf(obj_mgbf(1)%km_a_all,obj_mgbf(1)%nm,obj_mgbf(1)%mm))
------------------------^

Looks like the error is that the first MGBF ifdef in hybrid_ensemble_isotropic.F90 references USE_MGBF

 ! For MGBF
#ifdef USE_MGBF
   use mg_intstate
   use mg_timers
 #endif /* End of USE_MGBF block */

while all the other ifdefs reference USE_MGBF_def. We need to pick one.

My preference is to use USE_MGBF consistently through CMakeLists.txt and the source code. That said, I am not a cmake expert. Maybe the the two varaible approach of USE_MGBF & USE_MGBF_def is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Russ, Thanks for exploring on them. I think use of one variable or two variable is just a stylish problem. In this case, up to now, there are no actual differences. Using two variables ( as for USE_GSDCLOUD vs RR_CLOUDANALYSIS) just gives a reminder on how those directives in the source codes are being recognized by the compiler.
If it is ok with you, I still prefer to following the use of such a "two variable" approach in the previous GSI setup as for the above GSDCLOUD management.
If you think the "one variable" approach is better enough that we don't need use the same "two variable" approach in GSI for similar situations , I can change it later .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference remains to use one variable, USE_MGBF, all the way through. This is more readable than USE_MGBF_def and less confusing. You are the developer. It's your PR. Do as you wish.

type (mg_intstate_type), allocatable, dimension(:) :: obj_mgbf
#endif /* End of USE_MGBF_def block */
real(r_kind), allocatable, dimension(:,:,:) :: work_mgbf

! following is for special subdomain to slab variables used when internally generating ensemble members
Expand Down Expand Up @@ -1761,7 +1765,9 @@ subroutine destroy_ensemble
enddo
deallocate(ps_bar)
deallocate(en_perts)
#ifdef USE_MGBF_def
if(l_mgbf_loc) call print_mg_timers("mgbf_timing_cpu.csv", print_cpu, mype)
#endif /* End of USE_MGBF_def block */
end if
return

Expand Down Expand Up @@ -3735,6 +3741,8 @@ subroutine bkgcov_a_en_new_factorization(ig,a_en)
! because recursive filter is applied for ig>naensgrp
! to separate scales for scale-dependent localization
! even in MGBF-based localization)

#ifdef USE_MGBF_def
if(l_mgbf_loc.and.ig<=naensgrp) then

! Apply vertical smoother on each ensemble member
Expand Down Expand Up @@ -3774,6 +3782,7 @@ subroutine bkgcov_a_en_new_factorization(ig,a_en)
! Recursive/Spectral filter-based localization(ig<=naensgrp)
! or scale-separation(ig>naensgrp)
else
#endif /* End of USE_MGBF_def block */

! Apply vertical smoother on each ensemble member
! To avoid my having to touch the general sub2grid and grid2sub,
Expand Down Expand Up @@ -3824,7 +3833,9 @@ subroutine bkgcov_a_en_new_factorization(ig,a_en)
enddo
deallocate(a_en_work)

#ifdef USE_MGBF_def
endif
#endif /* End of USE_MGBF_def block */

return
end subroutine bkgcov_a_en_new_factorization
Expand Down Expand Up @@ -3892,6 +3903,7 @@ subroutine ckgcov_a_en_new_factorization(ig,z,a_en)
endif

! MGBF-based localization (now available only in regional=.true.)
#ifdef USE_MGBF_def
if(l_mgbf_loc) then

! Apply horizontal smoother for number of horizontal scales
Expand Down Expand Up @@ -3925,6 +3937,7 @@ subroutine ckgcov_a_en_new_factorization(ig,z,a_en)

! Recursive/Spectral filter-based localization
else
#endif /* End of USE_MGBF_def block */

if(grd_loc%kend_loc+1-grd_loc%kbegin_loc==0) then
! no work to be done on this processor, but hwork still has allocated space, since
Expand Down Expand Up @@ -3973,8 +3986,9 @@ subroutine ckgcov_a_en_new_factorization(ig,z,a_en)
call new_factorization_rf_z(a_en(k)%r3(ipnt)%q,iadvance,iback,ig)

enddo

#ifdef USE_MGBF_def
endif
#endif /* End of USE_MGBF_def block */

return
end subroutine ckgcov_a_en_new_factorization
Expand Down Expand Up @@ -4047,6 +4061,7 @@ subroutine ckgcov_a_en_new_factorization_ad(ig,z,a_en)
endif

! MGBF-based localization (now available only in regional=.true.)
#ifdef USE_MGBF_def
if(l_mgbf_loc) then

! Apply vertical smoother on each ensemble member
Expand Down Expand Up @@ -4080,6 +4095,7 @@ subroutine ckgcov_a_en_new_factorization_ad(ig,z,a_en)

! Recursive/Spectral filter-based localization
else
#endif /* End of USE_MGBF_def block */

! Apply vertical smoother on each ensemble member
iadvance=1 ; iback=2
Expand Down Expand Up @@ -4125,11 +4141,14 @@ subroutine ckgcov_a_en_new_factorization_ad(ig,z,a_en)
end if
end if

#ifdef USE_MGBF_def
endif
#endif /* End of USE_MGBF_def block */

return
end subroutine ckgcov_a_en_new_factorization_ad

#ifdef USE_MGBF_def
subroutine map_work_mgbf(f,g,iadvance,ig)
!$$$ subprogram documentation block
! . . .
Expand Down Expand Up @@ -4198,6 +4217,7 @@ subroutine map_work_mgbf(f,g,iadvance,ig)
return

end subroutine map_work_mgbf
#endif /* End of USE_MGBF_def block */

! ------------------------------------------------------------------------------
! ------------------------------------------------------------------------------
Expand Down Expand Up @@ -4538,13 +4558,15 @@ subroutine hybens_localization_setup
call normal_new_factorization_rf_z

if ( regional ) then ! convert s_ens_h from km to grid units.
#ifdef USE_MGBF_def
if ( l_mgbf_loc ) then
allocate(obj_mgbf(naensgrp))
do ig=1,naensgrp
write(mgbfname(9:10),'(i2.2)') ig
call obj_mgbf(ig)%mg_initialize(trim(mgbfname))
enddo
endif
#endif /* End of USE_MGBF_def block */
! Even for MGBF-localization, recursive filter is applied for scale-separation
! in scale-dependent localization, so init_rf_[xy] should be called in nsclgrp>1
if( .not. l_mgbf_loc .or. nsclgrp > 1 ) then
Expand Down Expand Up @@ -4767,13 +4789,17 @@ subroutine hybens_localization_setup
! nval_loc_en is the number of horizontally-filtered variables in the domain of each processor,
! which is the same as nval_lenz_en (horizontally-global and vertically-local) in recursive/spectral filter
! but horizontally-local and vertically-global in MGBF.
#ifdef USE_MGBF_def
if ( l_mgbf_loc ) then
nval_loc_en = maxval( obj_mgbf(1:naensgrp)%km_all &
& * (obj_mgbf(1:naensgrp)%im + obj_mgbf(1:naensgrp)%hx*2) &
& * (obj_mgbf(1:naensgrp)%jm + obj_mgbf(1:naensgrp)%hy*2) )
else
#endif /* End of USE_MGBF_def block */
nval_loc_en = nval_lenz_en
#ifdef USE_MGBF_def
endif
#endif /* End of USE_MGBF_def block */

! setup vertical weighting for ensemble contribution to psfc
call setup_pwgt
Expand Down
Loading