Keep floating point error in normalized weights from adding a replicate in weighted_quantile(n = ...) - #278
Open
haomeng797-ship-it wants to merge 1 commit into
Conversation
weighted_quantile(n = ...) expands each point into ceiling(weights * n) replicates. When the weights come from counts, normalizing and multiplying back by n can land a few ulp above a whole number, so a point picks up an extra replicate and the result no longer matches quantile() on the original sample, which is what n is documented to guarantee. Found while looking at mjskay#267.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while working through #267. This is a separate, narrower bug, so I've split it out rather than mixing it into that discussion.
The problem
The docs for
npromise that summarising a sample with duplicates into a weighted sample and then passing the original sample size gives backquantile()on the original sample. That breaks on some inputs:The weights are normalized to
weights / sum(weights), then expanded withn_rep = ceiling(weights * n). For the third group that round trip is not exact:so
ceiling()returns 8 instead of 7. That group gets an extra replicate, the total becomes 26 rather than 25, and everyp_kdownstream shifts.Sweeping 500 random integer samples across types 4–9 (3000 comparisons), 1.0% disagree with
quantile()on the original sample. The existing test for this correspondence happens to use counts4:1, whose normalization is exact, so it doesn't catch it.The fix
Shrink by a relative tolerance before rounding up, so weights that are exact multiples of
1/nstay put:pmax(1, ...)keeps very small weights from being shrunk to zero replicates, which would drop the point entirely. After the change the same 3000-comparison sweep has no mismatches.Tests
Added a regression test alongside the existing equivalence test, using counts that do trigger the rounding. It fails on
mainfor types 4, 5, 7, and 9, and passes with the fix.test.weighted_quantile.R,test.weighted_ecdf.R,test.weighted_hist.R, andtest.point_interval.Rall pass.Worth noting the tolerance is a pragmatic fix at the point of rounding, not a change to how weights are represented. If you'd rather have the expansion work from unnormalized weights (where counts stay exact), that would be a more structural fix, and I'm happy to redo it that way.