Skip to content

Fix rolling correctness edge cases - #61

Merged
kevinushey merged 11 commits into
masterfrom
bugfix/correctness-review
Sep 7, 2026
Merged

kevinushey merged 11 commits into
masterfrom
bugfix/correctness-review

Conversation

@kevinushey

@kevinushey kevinushey commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Summary

Rolling calculations could return incorrect results for extreme finite values, missing weighted products, and large strides. This PR fixes those cases while retaining ordinary arithmetic and incremental calculations where they are appropriate.

  • Validate window sizes, strides, logical controls, thread options, weight normalization, and variance frequency weights. Weights continue to override n, including an explicitly supplied NULL or invalid window size.
  • Make stride and fill geometry overflow-safe, recycle fill values consistently, and preserve factors in LOCF.
  • Preserve NA versus NaN behavior for extrema and the denominator of unnormalized weighted means across na.rm modes.
  • Avoid unnecessary overflow in means, median midpoints, variance centers, and frequency-weight totals, and preserve meaningful contributions from tiny frequency weights. Squared deviations can still overflow, even when the final SD is representable.
  • Retain guarded incremental products, forward multiplication for risky windows, and direct products for at most 16 outputs per column. Read unnormalized weights without a temporary copy.
  • Select ordinary variance and extremum kernels within existing work chunks, so input checks benefit from OpenMP and exceptional values only affect their own chunks. Preserve incremental crossovers and use the general kernels for widely spaced windows. Finalize ordinary mean lanes together while retaining overflow recovery.

Performance

The primary comparison now covers one million values with 5–99 nonuniform weights, plus missing values, unnormalized weights, matrices, strides, ten million values, and unweighted controls. Representative serial timings for 20 weights are:

Case PR base (ms) Previous PR (ms) Current (ms) Current / base
Sum 1.375 1.406 1.375 1.00x
Mean 1.406 2.219 1.531 1.09x
Min 3.313 10.781 2.125 0.64x
Max 3.344 10.813 2.125 0.64x
Var 10.750 64.125 11.250 1.05x
Sd 11.750 65.625 12.000 1.02x

Remaining costs are explicit: NA-heavy weighted means and extrema are still about 16–32% slower than the PR base in the serial comparison. The guarded unweighted product control also retains overhead. These should take priority over further rare-case optimization; this is not a blanket claim of regression-free performance.

See the common-workload report and reproduction instructions for all results, including two-thread and 14-thread comparisons. Earlier correctness/performance and short-result dispatch reports document the preceding changes and numerical constraints.

Validation

  • The extrema test reference preserves genuine NA operands before multiplication, avoiding platform-dependent payloads from NaN * NA. Explicit regressions cover both operand orders; both original failures were reproduced with an Intel build before correcting the reference.
  • Full testthat suites on serial and OpenMP builds, including new tests for arithmetic bounds, poorly centered windows, mixed ordinary/exceptional regions, normalization, NAs, signed zeros, integer inputs, matrices, fill, strides, and thread parity.
  • R CMD check --as-cran --no-manual: no errors or warnings; only the development-version metadata NOTE.
  • All 126 benchmark cases agree with the previous PR and base at tolerance 1e-8; six paired timing batches per case, with raw measurements retained. Builds, tests, and timing runs were kept separate.

@kevinushey
kevinushey merged commit 81b85b2 into master Sep 7, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant