Merge rrfs.v1.0.0 to develop#982
Conversation
This PR is to bring read_radar bug fix (NOAA-EMC#738) committed in the develop branch to RRFS release branch.
…age when compiling
NOAA-EMC#841) add GDIT's performance optimization for RRFS to release branch initialize variables in observer_fv3reg.f90 to avoid the warning message when compiling
…OAA-EMC#862) This is a draft PR to show fix for problems when running ctests using debug mode built GSI at rrfs.1.0.0.
This makes small changes needed for this branch to work with BUFR/12.2, which is the version being used for the RRFS implementation.
Add the GOES-19 satellite ID to the read-in list so that RRFSv1 can properly use abi_g19 data.
merge EMC repo/Release/rrfs.v1.0.0 to my fork
merging rrfs.v1.0.0 into test
|
ctest on wcoss2 100% tests passed, 0 tests failed out of 6 Total Test time (real) = 1925.22 sec |
|
ctest on ursa: 100% tests passed, 0 tests failed out of 6 |
HuiLiu-NOAA
left a comment
There was a problem hiding this comment.
Thanks.
I tested these modifications to the GSI develop/ branch in a retro DA test of conventional + AMV data, and the obs space diagnostics are identical to the RRFS release version/.
There was a problem hiding this comment.
Pull request overview
This PR merges the RRFS v1.0.0 release branch into develop, incorporating bug fixes, performance optimizations, and compatibility updates accumulated since the branch was frozen in March 2024. The changes include GDIT's performance optimizations using OpenMP parallelization and file-based caching, variable initialization fixes for debug mode, radar observation bug fix, and BUFR v12.1.0 compatibility updates.
Changes:
- Performance optimizations with OpenMP parallelization and node-local file caching for grid coefficient computation
- Variable initialization fixes to resolve debug mode CTest issues
- Integration of BUFR v12.1.0 compatibility changes
- Addition of timing diagnostics for ensemble member reading
- New CRC32 module for portable node identification
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gsi/read_iasi.f90 | Initializes data_all array to zero |
| src/gsi/mod_fv3_lola.f90 | Major performance optimization with OpenMP, file caching, and MPI node-aware broadcasting |
| src/gsi/hybrid_ensemble_isotropic.F90 | OpenMP parallelization and file caching optimization for normalization factors |
| src/gsi/gsi_files.cmake | Adds crc32 source files to build |
| src/gsi/get_gefs_for_regional.f90 | Adds MPI timing diagnostics |
| src/gsi/crc32_c.c | New C implementation of CRC32 using zlib |
| src/gsi/crc32.F90 | New Fortran module wrapping CRC32 for node identification |
| src/gsi/cplr_get_fv3_regional_ensperts.f90 | Adds timing diagnostics and OpenMP parallelization |
| src/gsi/combine_radobs.f90 | Initializes data_all_in array to zero |
| src/gsi/anisofilter.f90 | Moves operations inside mype==0 conditional to avoid unnecessary computation |
| src/gsi/CMakeLists.txt | Adds ZLIB dependency |
| src/enkf/loadbal.f90 | Initializes arrays to zero with commented debugging MPI barrier |
| src/enkf/enkf.f90 | Improves OpenMP thread safety using firstprivate to avoid false sharing |
| modulefiles/gsi_wcoss2.intel.lua | Updates module versions including BUFR and adds zlib |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sz' and deallocate this varaible
|
Reran CTest with the above modifications on wcoss2. 100% tests passed, 0 tests failed out of 6 Total Test time (real) = 1926.24 sec |
|
(base) [Shun.Liu@ufe01 build] ctest -j 6 100% tests passed, 0 tests failed out of 6 |
|
It seems the Copilot review caught some real issues which should be addressed. Other suggestions are for improving the robustness of the code - a good thing for operational codes. We need approvals from two reviewers not including members of the GSI Handling Review Committee, to move this PR forward. Hui approved. @TingLei-NOAA would you please review the changes in this PR? |
|
I think @daniel.kokron (the developers of this important optimization session) could give us a quick clarification/explanation on this issue . |
|
Test project /lfs/h2/emc/lam/noscrub/emc.lam/Shun.Liu/gsi_regression/GSI/build 100% tests passed, 0 tests failed out of 6 Total Test time (real) = 1925.61 sec |
|
Thank you @ShunLiu-NOAA for rerunning ctests. To save you time, ctests can wait until all conversations are resolved. |
Thank you, Russ. I just wanted to ensure that these minor changes I made have no impact on the results. |
|
@ShunLiu-NOAA : We can move this PR forward once we get feedback from Dan regarding the two remaining open conversations. Would you please follow up with Dan? |
| ! assume work load proportional to number of 'nearby' obs | ||
| call estimate_work_enkf1(numobs) ! fill numobs array with number of obs per horiz point | ||
| ! distribute the results of estimate_work to all processors. | ||
| ! added mpi_barrier for debug mode, on wcoss2. |
There was a problem hiding this comment.
@ShunLiu-NOAA Is this comment line still relevant? seems it should be removed to avoid confusing.
TingLei-NOAA
left a comment
There was a problem hiding this comment.
Now looks good to me!
RussTreadon-NOAA
left a comment
There was a problem hiding this comment.
Two peer reviews and approvals plus input from Dan Kokron.
Approve so @ShunLiu-NOAA can merge.
|
HERA ctest: 100% tests passed, 0 tests failed out of 6 Total Test time (real) = 1290.52 sec WCOSS2 ctest: 100% tests passed, 0 tests failed out of 6 Total Test time (real) = 1926.91 sec |
|
@ShunLiu-NOAA : I think this PR closes some GSI issues. I will close #981. What about #860. Does this PR also close this issue? Any other GSI issues we can close now that this PR has been merged? |
|
@RussTreadon-NOAA We can also close issue #860 (#860), as it should be resolved by the rrfs release branch. |
Description
The rrfsv1.0.0 branch was created in March 2024 when the RRFS code was frozen for pre-implementation testing. Since then, several updates have been applied to this branch:
Fixed a bug in read_radar. (#744)
Integrated GDIT's performance optimizations for RRFS. (#841)
Initialized variables in observer_fv3_reg.f90.
Resolved issues with CTest when running in debug [mode.] (#862)
Updated bufr_d to bufr_4 for compatibility with BUFR v12.1.0. (#963)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
These changes were tested in Realtime RRFS workflow by EMC and NCO.
Checklist