Skip to content

Feature #3426 obs_error_improvements - #3438

Merged
JohnHalleyGotway merged 16 commits into
developfrom
feature_3426_obs_error_improvements
Sep 11, 2026
Merged

JohnHalleyGotway merged 16 commits into
developfrom
feature_3426_obs_error_improvements

Conversation

@JohnHalleyGotway

@JohnHalleyGotway JohnHalleyGotway commented Sep 4, 2026 •

Copy link
Copy Markdown
Collaborator

Expected Differences

Note that I included additional changes for this PR on September 9th to address SonarQube issues:

  • Importantly, the SonarQube scan has been failing since the switch to using METbaseimage version 13 last week. The changes in build_met_sonarqube.sh fix it and increase the total number from ~2K to ~14K where it really should be.
  • Squashing some SonarQube findings caused others to pop up. So I backed out some recent changes with an eye toward simplicity. At this point, I'm comfortable with the findings that remain for this PR. If you don't pick a good place to stop, the SonarQube work would never end.
  • In total the SonarQube findings are reduced very slightly from 14,087 in develop to 14,082 for this PR.

These changes provide an overhaul to the observation error logic to make its application more efficient. This work was heavily assisted by AI. Listed below is a description of the changes:

Summary

Fixes #3426. Applying observation error corrections and perturbations in Ensemble-Stat was severely degrading runtime performance — a 20-member HAFS ensemble case went from ~1 minute to multiple hours with obs_error enabled. This PR profiles and addresses the actual bottlenecks in vx_statistics/obs_error.{h,cc} and its callers.

Changes

ObsErrorTable::lookup() was the dominant cost. Every call linearly scanned the full table and recompiled a POSIX regex against every row (var_name matching), for every grid point or observation. Fixed with:

  • A per-variable-name subset cache (ObsErrorTable::var_subset()) so repeat lookups for the same variable only scan the rows that could ever match, instead of the whole table.
  • A last-matched-row cache, since adjacent grid points/observations very often resolve to the same entry (e.g. the same VAL_RANGE bucket).

Redundant lookups eliminated. When a table entry depends on VAL_RANGE (true for every APCP_* row in the default table), Ensemble-Stat was repeating the table lookup independently for every ensemble member at every grid point (add_obs_error_inc()), then again while building the pair data — up to n_members + 1 lookups per point. build_obs_error_entry_grid() now resolves each point once and caches it for reuse across all members and downstream processing.

Early-exit checks added. New ObsErrorEntry::need_bias_correction() / need_perturbation() predicates let add_obs_error_bc()/add_obs_error_inc() skip the entire per-point loop when an entry does nothing — which is always true for bias correction against the shipped default table (no row sets INST_BIAS_SCALE/INST_BIAS_OFFSET).

OpenMP parallelization, with raw-buffer access (DataPlane::data()/buf()) replacing bounds-checked get()/set() calls in the hot loops. Since perturbation draws from a GSL random number generator, a new rng_set_omp()/rng_free_omp() (in vx_gsl_prob/gsl_randist) clones one independent RNG per thread. The original grid traversal order is preserved so that single-threaded output (OMP_NUM_THREADS unset or 1, the MET default) stays bit-for-bit identical to before this change; output only changes, as expected for a parallel RNG, once a user explicitly requests OMP_NUM_THREADS > 1.

Removed the unused gsl_rng* parameter from add_obs_error_bc() — bias correction never touched the RNG.

Logging cleaned up. A recently-added per-point warning on failed table lookups (#3429/#3431) could itself flood the log on gridded data with many non-matching points. Replaced with Debug(4) (opt-in detail) plus one aggregated Debug(2) "Skipping N of M" summary per call site.

Test updates: unit_ensemble_stat.xml's OBSERR/OBSERR_BAD_LOOKUP tests now pin OMP_NUM_THREADS=1, since CI's default test environment sets OMP_NUM_THREADS=$(nproc) for all unit tests — without pinning it, the (correct, expected) parallel RNG behavior would diverge from the existing serial truth data for these two tests specifically.

  • 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

  • Describe testing already performed for these changes:

  • Full --enable-all build verified clean.

  • Before/after regression: built the pre-change and post-change code separately and diffed all ensemble_stat_OBSERR* output files (.stat, _ecnt.txt, _rhist.txt, _phist.txt, _relp.txt, _ssvar.txt, _orank.txt/.nc) against real unit-test data with OMP_NUM_THREADS unset — output is byte-for-byte identical.

  • Confirmed the aggregated lookup-failure logging behaves correctly against a deliberately truncated obs-error table.

  • Manually ran the 20-member HAFS example with OMP_NUM_THREADS=10 with only a 2.5 minute runtime!

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

  • Review code changes. Consider testing ensemble_stat in:

    • seneca:/d1/projects/MET/MET_pull_requests/met-13.0.0/rc1/MET-feature_3426_obs_error_improvements/bin/ensemble_stat
    • casper:/glade/work/johnhg/METplus/development/METplus-13.0/rc1/MET-feature_3426_obs_error_improvements/bin/ensemble_stat
  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]
    None needed.

  • Do these changes include sufficient testing updates? [Yes]
    Set OMP_NUM_THREADS =1 to get the same test output as before.

  • 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? [Yes or No]

    If yes, create a new Update Truth METplus issue to describe them.
    It might. If observation error in ensemble_stat is being exercised and OMP_NUM_THREADS > 1, then changes to the random number generator could modify the results.

  • Do these changes introduce new SonarQube findings? [Yes or No]

    If yes, please describe:

  • Please complete this pull request review by [Fri, Sept 11, 2026?].

Pull Request Checklist

See the METplus Workflow for details.

  • Review the source issue metadata (required labels, projects, and milestone).
  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    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
  • After submitting the PR, select the ⚙️ icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

@JohnHalleyGotway JohnHalleyGotway added this to the MET-13.0.0 milestone Sep 4, 2026
@github-project-automation github-project-automation Bot moved this to 🩺 Needs Triage in METplus-13.0 Development Sep 4, 2026
@JohnHalleyGotway JohnHalleyGotway moved this from 🩺 Needs Triage to 🔎 In review in METplus-13.0 Development Sep 4, 2026
@JohnHalleyGotway
JohnHalleyGotway marked this pull request as ready for review September 8, 2026 18:12
…ed for a cast that was flagged by SonarQube.
…y. Also restructure logic to avoid too many nested loops.
…appeared on Sept 4, 2026. The number of findings dramatically decreased from ~13K to ~2K which is likely a SonarQube configuration issue.
…ig issue. First, after switching to METbaseimage:13.0, the make step failed because we were forcing MET_CXX_STANARD=11. However, that build script failed to check the return status and so it looked like the scan succeeded. This led to an apparent reduction of SonarQube findings from ~13k to ~2k. The changes here are to check the return status so that the SonarQube job fails when the compilation fails. Also, remove MET_CXX_STANDARD=11 from the configuration so that the SonarQube build will now succeed and we'll get back up to ~13k findings.
…ic 3 levels deep. Those changes caused other SonarQube findings about the number of function arguments being greater than 7. The previous implementation was simpler and more extensive changes are needed to avoid nesting > 3 while keeping function arguments <= 7.
@sonarqubecloud

Copy link
Copy Markdown

@CPKalb CPKalb left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Talked with John about SonarQube findings. Also, tested on my model runs and verified that the run was significantly faster while producing the same output.

@JohnHalleyGotway
JohnHalleyGotway merged commit 0652387 into develop Sep 11, 2026
41 of 42 checks passed
@github-project-automation github-project-automation Bot moved this from 🔎 In review to 🏁 Done in METplus-13.0 Development Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🏁 Done

Development

Successfully merging this pull request may close these issues.

Improve Ensemble-Stat to more efficiency apply observation error corrections and perturbations

2 participants