Feature #3145 openmp apps #3146
Conversation
…o most MET tools via regridding.
… threads that actually are available.
… in the vx_regrid library to avoid duplication.
… using stl::vector's instead of manual memory management. This required introducing a DataPlaneArray::at(int) accessor function. Modify some library and application code to get things compiling.
…ike apply_mask(). Doing so produced a Grid-Stat runtime failure.
…n all GHA tests more efficiently.
… parallelized regridding actually runs well and produces the expected output.
…nto feature_3132_openmp_vx_util
…runtime segfault in the unit_regrid.xml unit test.
…es that include break statements.
…he compilation.
… the maximum number of available threads. ci-run-unit
…nce that triggers a failure when ncdiff is used to compute differences: libgomp: Invalid value for environment variable OMP_NUM_THREADS ... ci-run-unit
…te unit_met_test_scripts.xml to add missing expected output file names. ci-run-unit
…uction(+:n) to avoid race conditions updating the counter. ci-run-unit
…o feature_3134_openmp_libs
…r incrementing counts in vector<long long>. Note that no diffs were noted on seneca with OMP_NUM_THREADS > 1, but when run on 4 threads via GHA, we did get a slight reduction in the PDF counts. Need to confirm that this reduction actually fixes the problem via GHA. ci-run-unit
… integers to NetCDF output files. Just casting using (float *) or (int *) did NOT work. Instead, provide utility functions in vx_nc_util and update the application code to call those utility functions.
…E_CTS TOTAL output column.
…et rid of some dynamically allocated memory.
…tting as there was some uncertainty about what's actually be used by GitHub.
There was a problem hiding this comment.
@JohnHalleyGotway Thanks for outlining the code changes and the timing of the unit test commands. I have reviewed the code changes, confirmed no diffs were flagged by the GHA testing workflow, and confirmed that the overall number of SonarQube code smells are reduced by 243. Thanks for all of your work on these modifications and improvements. I approve this request.
There was a problem hiding this comment.
I tested this PR using both grid_stat and MODE to verify RRFS output against MRMS radar reflectivity.
For grid_stat, I used three masks: CONUS, EAST, and WEST.
time grid_stat mpashn4nssl_2024051612f06.grib2 MergedReflectivityQCComposite_00.50_20240516-180000.grib2 GridStatConfig -outdir output
For MODE, a total of 76 forecast objects and 48 observation objects were identified.
time mode mpashn4nssl_2024051612f06.grib2 MergedReflectivityQCComposite_00.50_20240516-180000.grib2 MODEConfig -outdir output
I used the following builds:
- Reference:
/d1/projects/MET/MET_regression/develop/NB20250515/MET-develop/bin - PR Test:
/d1/projects/MET/MET_pull_requests/met-12.1.0/rc1/MET-feature_3134_openmp_libs/bin
Tests were performed with and without OMP_NUM_THREADS, using values of 1, 2, 4, 8, 16, 32, and 40.
GridStat Timing Results:
| Threads | Reference | PR |
|---|---|---|
| 1 | 1m13.594s | 1m13.600s |
| 2 | 0m44.448s | 0m41.015s |
| 4 | 0m24.467s | 0m24.991s |
| 8 | 0m16.068s | 0m16.026s |
| 16 | 0m11.813s | 0m11.893s |
| 32 | 0m10.686s | 0m10.644s |
| 40 | 0m10.173s | 0m10.382s |
MODE Timing Results:
| Threads | Reference | PR |
|---|---|---|
| 1 | 10m18.787s | 10m19.344s |
| 2 | 6m41.242s | 6m39.983s |
| 4 | 4m51.949s | 4m50.502s |
| 8 | 3m48.974s | 3m49.537s |
| 16 | 3m19.108s | 3m18.730s |
| 32 | 3m22.185s | 3m21.948s |
| 40 | 3m18.661s | 3m20.209s |
Based on the results of my testing, functionality is preserved and performance remains stable or improved. Output from both grid_stat and MODE was consistent between the PR and reference builds. Performance scaling with OpenMP is maintained, and the timing differences observed were minimal and within expected variability. I approve this PR.
…r/MET into feature_3145_openmp_apps
|
Thanks @briannen for the review. @jprestop can you please take a look at these updated docs? |
Note that this feature branch includes changes from
feature_3134_openmp_libsfrom PR #3141. That PR should be approved and merged before this PR is reviewed.After PR #3141 is merged, this PR will propose changes in 23 files:
test_env_vars.shlog the number of threads used to make it easy to check via GitHub actions.data_plane.haddconst_buf()member function to access a "const" version of the data.nc_utils.h/.ccadd utility functions for writing the contents ofDataPlaneobjects to NetCDF output files.write_netcdf.hno real changes, just consistent formatting.shapedata.ccreduce SonarQube code smells.grid_stat.ccparallelizes the application of masks in 2 loops and reduce SQ code smells.combine_boolplanes.cc,mode_exec.cc,mode_ps_file.cc,mode_superobject.cc,multivar_data.cc,objects_from_netcdf.cc) parallelizes some loops and reduces SQ code smells..pcp_combine.cc,series_analysis.cc,gen_vx_mask.cc,regrid_data_plane.cc, andtc_gen.cceliminate loops when writing output by calling newnc_utils.h/.ccfunction.wavelet_stat.ccparallelizes at least 7 loops.gen_ens_prod.cc,point2grid.cc,shift_data_plane.cc, andtc_dland.ccparallelizes a handful of loops and eliminates some by calling newnc_utils.h/.ccfunction.Expected Differences
Do these changes introduce new tools, command line arguments, or configuration file options? [No]
If yes, please describe:
Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No]
If yes, please describe:
Pull Request Testing
Tested the timing of the following unit test commands. All scripts can be found in
seneca:/d1/projects/MET/MET_pull_requests/met-12.1.0/rc1/MET-feature_3145_openmp_apps/internal/test_unit.time unit_mode_multivar.shvx_regridlibrary.time unit_grid_stat.shtime unit_wavelet_stat.shtime unit_gen_ens_prod.shtime unit_point2grid.shtime unit_shift_data_plane.shTest:
time unit_tc_dland.shRuntime: (1) 1m22.068s, (16) 0m7.768s, (32) 0m5.212s
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Review code changes.
Confirm no diffs are flagged by the GHA testing workflow (after PR Feature #3134 openmp libs #3141 is merged).
Confirm that the overall number of SonarQube code smells are reduced.
Please find code for this PR compiled in:
seneca:/d1/projects/MET/MET_pull_requests/met-12.1.0/rc1/MET-feature_3145_openmp_apps.Some of these improvements are minor while others more substantial. As long as performance is not degraded, I'd recommend accepting them.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
None needed.
Do these changes include sufficient testing updates? [Yes]
None needed.
Will this PR result in changes to the MET test suite? [No]
If yes, describe the new output and/or changes to the existing output:
Will this PR result in changes to existing METplus Use Cases? [No]
If yes, create a new Update Truth METplus issue to describe them.
Do these changes introduce new SonarQube findings? [No]
If yes, please describe:
The overall number of code smells are reduced by 243, from 16,792 in develop to 16,549 in this feature branch. I reviewed/fixed all of the "easy" code smells that we're flagged as "New Code".
Please complete this pull request review by [Fri 5/16/25].
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: METplus-X.Y Support project for bugfix releases or MET-X.Y Development project for the next coordinated release