Fix NA density value when a bound coincides with an extremum of x - #277
Open
haomeng797-ship-it wants to merge 1 commit into
Open
Fix NA density value when a bound coincides with an extremum of x#277haomeng797-ship-it wants to merge 1 commit into
haomeng797-ship-it wants to merge 1 commit into
Conversation
The trimming step interpolates onto seq(min(x), max(x)), but when a bound equals the matching extremum the internal grid can fall a few ulp short of it, so the endpoint lands outside the interpolation range and comes back NA. That NA then propagates into hdi() and mode_hdi(), which error. Closes mjskay#269
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.
Closes #269.
The cause
density_bounded()trims by interpolating the reflected density ontoseq(min(x), max(x), length.out = n):d$xis a slice of the wider gridseq(bounds[[1]] - width, bounds[[2]] + width, length.out = n_unbounded), so its endpoints reproduceboundsonly up to floating point error. When a bound happens to equal the corresponding extremum ofx, that error is enough to break the interpolation:x_trimmedends exactly atmax(x),d$xends a couple of ulp below it, andapprox()returnsNAfor that last point.In @ASKurz's reprex the discovered upper bound is exactly
max(x) = 1, and the grid lands at0.99999999999999978, short by2.22e-16. That singleNAthen reachesquantile()insidehdi(), which is where the error surfaces:A smaller deterministic reproducer, no sampling involved:
The fix
approx(..., rule = 2). Thetrimbranch only runs whenboundsalready containsrange(x), so anything outside the interpolation range is floating point noise at the endpoints, and constant extrapolation over a few ulp is exact to the precision available. This leaves the returnedxgrid unchanged, sotrimstill means "the output spans exactly the range of the data".I considered clamping
x_trimmedtorange(d$x)instead, but that would make the returned grid stop just short ofmax(x)and quietly weaken whattrimpromises.Tests
Added a regression test using the deterministic reproducer above. It fails on
mainwith both theNAand thehdi()error, and passes with the fix.test.density.Randtest.point_interval.Rpass; the remaining failures in the full suite are the pre-existing vdiffr snapshot differences from #272, which also fail on a clean checkout here.One thing I noticed while testing, not addressed here since it is a separate question: on the reprex's
p^3posterior, whose true density is monotone,hdi()now returns two disjoint intervals rather than one. That looks like ordinary noise in the density estimate creating a shallow local minimum, not a bug in this code path, but it may be worth a look if you think the density should be smooth enough there to avoid splitting.