GitHub Issue NOAA-EMC/GSI#450 Using global and regional ensembles simultaneously for fv3-lam GSI EnVar (add l_both_fv3sar_gfs_ens option) #451
Conversation
MichaelLueken
requested review from
MichaelLueken,
BinLiu-NOAA,
dtkleist,
hu5970 and
lbi2018
| @@ -242,8 +257,10 @@ subroutine get_fv3_regional_ensperts_run(this,en_perts,nelen,ps_bar) | |||
| kapr=one/rd_over_cp | |||
| ! | |||
| ! LOOP OVER ENSEMBLE MEMBERS | |||
| do n=1,n_ens | |||
| write(ensfilenam_str,22) trim(adjustl(ensemble_path)),ens_fhrlevs(m),n | |||
| !cltorg do n=1,n_ens | |||
There was a problem hiding this comment.
Remove line 260 is unused.
| @@ -493,7 +510,8 @@ subroutine get_fv3_regional_ensperts_run(this,en_perts,nelen,ps_bar) | |||
| enddo | |||
| ! | |||
| ! CALCULATE ENSEMBLE MEAN | |||
| bar_norm = one/float(n_ens) | |||
| !cltorg bar_norm = one/float(n_ens) | |||
There was a problem hiding this comment.
Remove unused comment line
|
|
||
| do n=1,n_ens | ||
| !cltorg do n=1,n_ens |
src/gsi/get_gefs_for_regional.f90
Outdated
| @@ -42,6 +42,7 @@ subroutine get_gefs_for_regional | |||
| use hybrid_ensemble_parameters, only: region_lat_ens,region_lon_ens | |||
| use hybrid_ensemble_parameters, only: en_perts,ps_bar,nelen | |||
| use hybrid_ensemble_parameters, only: n_ens,grd_ens,grd_a1,grd_e1,p_e2a,uv_hyb_ens,dual_res | |||
| use hybrid_ensemble_parameters, only: n_ens_gfs | |||
| use hybrid_ensemble_parameters, only: full_ensemble,q_hyb_ens,l_ens_in_diff_time,write_ens_sprd | |||
| use hybrid_ensemble_parameters, only: ntlevs_ens,ensemble_path,jcap_ens | |||
| !use hybrid_ensemble_parameters, only: add_bias_perturbation | |||
There was a problem hiding this comment.
Hi @TingLei-daprediction. I have completed my review. The code was compiled using Intel and GNU compilers without issue. While compiling in debug mode, one variable was identified as unused (n_ens on line 44 of get_gefs_for_regional.f90). The regression tests were run for the Intel, GNU, and debug executables and all passed successfully. There were a few issues I've noted in my review (@lbi2018 has also noted several of these in her review). Before making any changes to the source code, please update your branch to the latest authoritative repository using:
- git remote add upstream https://github.com/NOAA-EMC/GSI.git
- git remote update
- git rebase upstream/develop
Please correct the conflicts in src/gsi/gsimod.F90 and src/gsi/hybrid_ensemble_parameter.f90.
- git rebase --continue
Please apply the changes from @lbi2018 and my reviews.
- git push origin feature/gsi_fv3lam_mix_ens --force
If you have any questions or encounter any issues, please let me know.
TingLei-daprediction
force-pushed
the
feature/gsi_fv3lam_mix_ens
branch
from
5196e82
to
95d4d29
Compare
|
@lbi2018 and @MichaelLueken-NOAA I had finished the changes as requested and pushed them back to the repository. |
|
Hi @TingLei-daprediction. I see that you have merged the latest authoritative repository to your |
TingLei-daprediction
force-pushed
the
feature/gsi_fv3lam_mix_ens
branch
from
95d4d29
to
2b9af42
Compare
|
@MichaelLueken-NOAA , My bad. the local changes were not committed back. Now it should be ok. Thanks. |
There was a problem hiding this comment.
@TingLei-daprediction Thank you very much for addressing @lbi2018 and my concerns! On lines 234 and 235 of src/gsi/get_gefs_for_regional.f90, if these lines should be kept (they appear to be debug writes, at first glance), then please indent these two lines two spaces so that they align with line 238 below. Once completed and once @lbi2018 has given her approval, I will be able to submit your work to the review committee. Thanks again!
TingLei-daprediction
force-pushed
the
feature/gsi_fv3lam_mix_ens
branch
from
2b9af42
to
5e6ba1d
Compare
…aneously for fv3-lam GSI EnVar (add l_both_fv3sar_gfs_ens option)
TingLei-daprediction
force-pushed
the
feature/gsi_fv3lam_mix_ens
branch
from
5e6ba1d
to
7465c3e
Compare
|
@MichaelLueken-NOAA Thanks for pointing me to this line, around which I should give more careful treatment with more informative warning as the current version. |
|
Hi @lbi2018. At your earliest convenience, please go through the changes again and make sure that you are happy with the changes. If you are, please approve the changes, otherwise, please let @TingLei-daprediction know. Thank you very much! |
|
Approved the changes. Thanks. -Li |
MichaelLueken
changed the title
|
The due date for these changes have past with no feedback, so I will give final approval to these changes and merge them to the authoritative develop branch. |
This PR is to add the mixed ensemble option for fv3-lam GSI EnVar. #450.
It passed the developer's own verification tests for this branch on hera. On hera, It failed in a few GSI 's own regression tests for the scalability problem, which I think could be attributed to some hera's system performance change and could be disregard now. I am re-running those regression tests and would update here.
@MichaelLueken-NOAA I did the rebase for this change and please let me know if it doesn't meet the GSI PR requirement.