fix(density): order-independent neighbourhood radius + reusable DensityEstimator#414
Open
agene0001 wants to merge 2 commits into
Open
fix(density): order-independent neighbourhood radius + reusable DensityEstimator#414agene0001 wants to merge 2 commits into
agene0001 wants to merge 2 commits into
Conversation
…t element
`knn_pdf` and `kde_pdf` read the neighbourhood radius as
`neighbors.last().unwrap()`, relying on an ordering `KdTree::within` never
documented. It happened to hold in kdtree 0.7, which returned
`evaluated.into_sorted_vec()`; 0.8 returns raw `BinaryHeap` order, so the last
element is an arbitrary neighbour and both densities were scaled by the wrong
ball volume. `nearest()` still sorts in 0.8, which is why only the
`Some(bandwidth)` path was affected.
`nearest_neighbors` now returns a `NearestNeighbors { squared_distances, k,
radius }` whose `radius` is computed as an explicit maximum, and whose docs state
that the distance order is unspecified. Verified under both 0.7.0 and 0.8.1, and
the kdtree requirement is moved to 0.8.1 (Cargo.lock.MSRV regenerated with
`cargo hack check --rust-version`).
The Monte Carlo tests could not reliably catch this, so it is pinned by a
deterministic one: 7 hand-placed samples, query at 0, squared radius 4 gives
exactly k = 5, radius = 2, and `knn_pdf == 5/28`. Reading an arbitrary
neighbour's distance instead would give 5/14.
Adds `DensityEstimator`, which owns the k-d tree so it can be built once and
queried many times. `nearest_neighbors` used to rebuild the whole tree on every
call - about 59% of a single `knn_pdf` at n = 1e5:
200-point grid, n = 10_000 32.4 ms -> 4.4 ms (6.8x)
200-point grid, n = 100_000 305.8 ms -> 44.1 ms (6.9x)
kde_pdf, 20 points, n = 1e5 35.1 ms -> 7.4 ms (4.8x)
`knn_pdf`/`kde_pdf` are kept as thin wrappers, and a test asserts the prepared
estimator is bit-identical to them over 40 grid points and 3 bandwidths. The tree
dimension now comes from the samples rather than from a query point, so a
mismatched query is rejected instead of building a wrong-shaped tree.
`[`DensityEstimator`](crate::density::DensityEstimator)` resolves to the same item as `[`DensityEstimator`]`, so the explicit target trips `rustdoc::redundant_explicit_links` under `RUSTDOCFLAGS=-D warnings`. Not a CI failure: no workflow runs `cargo doc`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Two things, one file family (
src/density/):1. A latent ordering bug.
knn_pdf/kde_pdfread the neighbourhood radius asneighbors.last().unwrap(), relying on an orderingKdTree::withinnever documented. kdtree 0.7 happened to returninto_sorted_vec(); 0.8 returns rawBinaryHeaporder, so the last element is an arbitrary neighbour and both densities get scaled by the wrong ball volume. (nearest()still sorts in 0.8, which is why only theSome(bandwidth)path broke.)nearest_neighborsnow returnsNearestNeighbors { squared_distances, k, radius }withradiuscomputed as an explicit maximum, documented as order-independent. Verified under both 0.7.0 and 0.8.1; the requirement moves to 0.8.1 andCargo.lock.MSRVis regenerated withcargo hack check --rust-version.The Monte Carlo tests couldn't reliably catch this, so it's pinned deterministically: 7 hand-placed samples, query at 0, squared radius 4 ⇒ exactly k = 5, radius = 2,
knn_pdf == 5/28. Reading an arbitrary neighbour's distance gives 5/14.2.
DensityEstimator: build the tree once.nearest_neighborsrebuilt the k-d tree on every call — ~59% of a singleknn_pdfat n = 1e5. The estimator owns the tree:DensityEstimatorknn_pdf, n = 10⁴knn_pdf, n = 10⁵kde_pdf(20 pts), n = 10⁵knn_pdf/kde_pdfremain as thin wrappers, and a test asserts the estimator is bit-identical to them over 40 grid points × 3 bandwidths. The tree dimension now comes from the samples rather than the query point, so a mismatched query is rejected instead of silently building a wrong-shaped tree. Adensity_grid_200criterion group tracks the rebuild-vs-prepared comparison.🤖 Generated with Claude Code