Kd tree peer finding #2
Merged
Conversation
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
…ltidimensional points (performance weighted route discovery)
…rs, and update version to 0.3.0
… CI configuration
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
…n KDTree functions
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This commit introduces a comprehensive test suite for the `gonum` backend, which was previously untested. It also adds tests for the `kdtree_helpers` package, specifically for the `ComputeNormStats3D` and `BuildND` functions. The new tests cover a wide range of scenarios, including: - Basic functionality of `Nearest`, `KNearest`, and `Radius` - Edge cases such as empty trees, zero/negative inputs, and mismatched dimensions - Various data configurations, including collinear points and negative coordinates This commit also includes minor fixes to the existing tests to improve their robustness and accuracy. As a result of these changes, the overall test coverage of the project has been increased from 80% to over 90%.
feat: Increase test coverage to over 90%
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
kdtree_gonum_test.go(1 hunks)kdtree_helpers_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
kdtree_gonum_test.go (1)
kdtree.go (8)
KDPoint(25-29)NewKDTree(225-270)WithBackend(203-203)BackendGonum(194-194)WithMetric(198-198)BackendLinear(193-193)ErrEmptyPoints(11-11)NewKDTreeFromDim(274-294)
kdtree_helpers_test.go (2)
kdtree_helpers.go (12)
Build2D(225-258)Build3D(288-329)Build4D(363-412)ComputeNormStats2D(173-183)Build2DWithStats(261-285)NormStats(29-31)AxisStats(22-25)Build3DWithStats(332-360)ComputeNormStats4D(202-218)Build4DWithStats(415-447)ComputeNormStats3D(186-199)BuildND(71-89)kdtree.go (1)
NewKDTree(225-270)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-test-gonum
- GitHub Check: build-test-wasm
- GitHub Check: build-test-wasm
- GitHub Check: build-test-gonum
🔇 Additional comments (24)
kdtree_helpers_test.go (8)
8-33: LGTM!The test correctly validates normalization, inversion, and weighting logic. The inline comments clearly explain the expected coordinate transformations.
35-62: LGTM!The test correctly validates the min==max edge case where scale01 returns 0. The constant ID is acceptable since this test doesn't construct a KDTree that would enforce uniqueness.
65-112: LGTM!Excellent end-to-end test that validates the complete workflow from 4D building through normalization, weighting, inversion, to nearest-neighbour queries. The comment on line 108 helpfully documents the expected outcome.
114-154: LGTM!This parity test correctly validates that Build2D and Build2DWithStats produce identical coordinates, ensuring consistency between the automatic and explicit stats paths.
156-178: LGTM!Correctly validates that Build3DWithStats handles min==max axes by producing zero coordinates, consistent with the scale01 function behaviour.
227-245: LGTM!The test correctly validates ComputeNormStats3D. The manual field comparison is clear and appropriate for this simple structure.
247-267: LGTM!The test correctly validates BuildND's happy path with 3 dimensions, checking both point count and dimensionality.
269-283: LGTM!The test correctly validates that BuildND returns an error when the weights slice length doesn't match the number of extractors.
kdtree_gonum_test.go (16)
1-8: LGTM!The build tag, package declaration, and imports are correct. The build tag ensures these tests only run when the Gonum backend is available.
10-20: LGTM!The
equalishhelper correctly implements tolerance-based floating-point comparison.
53-69: LGTM!Basic smoke test for
Nearestwith the Gonum backend is correct.
71-92: LGTM!Basic smoke test for
KNearestwith the Gonum backend is correct.
94-115: LGTM!Basic smoke test for
Radiuswith the Gonum backend is correct.
117-128: LGTM!Correctly verifies that unsupported metrics cause a fallback to the linear backend.
137-142: Testing internal implementation details.This test directly calls the internal
axisStdfunction. IfaxisStdis not part of the public API, this test couples your test suite to internal implementation details. Consider whether this behaviour is adequately exercised through public API tests, or whetheraxisStdshould be exported if it requires direct testing.
152-165: LGTM!Correctly verifies that dimension mismatches are handled gracefully.
181-193: LGTM!Correctly verifies that zero-K queries return empty results.
195-207: LGTM!Correctly verifies that negative radius queries return empty results.
209-221: LGTM!Correctly verifies behaviour with a single-point tree.
238-254: LGTM!Correctly verifies KNearest with k=1.
255-264: LGTM!Correctly tests empty tree behaviour using the public API.
265-275: Testing internal implementation details.This test directly calls the internal
buildKDRecursivefunction. Consider whether this behaviour is adequately covered by public API tests, such as constructing trees with single points and querying them.
277-291: LGTM!Correctly verifies that KNearest returns all available points when k exceeds the tree size.
292-625: Comprehensive test coverage.The remaining test functions provide excellent coverage of various edge cases and scenarios:
- Identical and overlapping points
- Various distance scenarios
- Collinear points
- Negative coordinates
- Extreme float values
- Multi-dimensional inputs
- Ordering guarantees
This thorough testing will help ensure the Gonum backend behaves correctly.
Comment on lines
+22
to
+51
| func TestGonumKnnHeap(t *testing.T) { | ||
| h := knnHeap{} | ||
|
|
||
| h.push(knnItem{idx: 1, dist: 1.0}) | ||
| h.push(knnItem{idx: 2, dist: 2.0}) | ||
| h.push(knnItem{idx: 3, dist: 0.5}) | ||
|
|
||
| if h.Len() != 3 { | ||
| t.Errorf("expected heap length 3, got %d", h.Len()) | ||
| } | ||
|
|
||
| item := h.pop() | ||
| if item.idx != 2 || item.dist != 2.0 { | ||
| t.Errorf("expected item with index 2 and dist 2.0, got idx %d dist %f", item.idx, item.dist) | ||
| } | ||
|
|
||
| item = h.pop() | ||
| if item.idx != 1 || item.dist != 1.0 { | ||
| t.Errorf("expected item with index 1 and dist 1.0, got idx %d dist %f", item.idx, item.dist) | ||
| } | ||
|
|
||
| item = h.pop() | ||
| if item.idx != 3 || item.dist != 0.5 { | ||
| t.Errorf("expected item with index 3 and dist 0.5, got idx %d dist %f", item.idx, item.dist) | ||
| } | ||
|
|
||
| if h.Len() != 0 { | ||
| t.Errorf("expected heap length 0, got %d", h.Len()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Testing internal implementation details.
This test directly accesses the internal knnHeap type. Whilst thorough unit testing of internal structures can be valuable, consider whether this behaviour is sufficiently covered by the public API tests. If knnHeap is meant to remain internal, ensure changes to its implementation won't break these tests unnecessarily.
Additionally, this test is largely duplicated by TestGonumKnnHeapPop (lines 222-236). Consider consolidating or removing the duplicate.
🤖 Prompt for AI Agents
In kdtree_gonum_test.go around lines 22-51, the test inspects the internal
knnHeap type and duplicates behavior already covered by TestGonumKnnHeapPop;
update the suite by either (A) removing these lines and relying on the existing
TestGonumKnnHeapPop to cover heap behavior, or (B) rewriting this test to
exercise the public API (call the exported KNN/search functions) instead of
directly using knnHeap so internal implementation changes won't break tests; if
you keep both, consolidate assertions into a single test to avoid duplication
and ensure the remaining test asserts identical expected ordering and length
checks.
Comment on lines
+130
to
+179
| func TestGonumNearestWithEmptyTree(t *testing.T) { | ||
| _, err := NewKDTree([]KDPoint[int]{}, WithBackend(BackendGonum)) | ||
| if err != ErrEmptyPoints { | ||
| t.Fatalf("expected ErrEmptyPoints, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestAxisStdWithNoPoints(t *testing.T) { | ||
| stds := axisStd(nil, nil, 2) | ||
| if len(stds) != 2 || stds[0] != 0 || stds[1] != 0 { | ||
| t.Errorf("expected [0, 0], got %v", stds) | ||
| } | ||
| } | ||
|
|
||
| func TestGonumNearestWithNilRoot(t *testing.T) { | ||
| backend := &kdBackend{root: nil, dim: 2} | ||
| _, _, ok := gonumNearest[int](backend, []float64{1, 1}) | ||
| if ok { | ||
| t.Error("expected no point found, but got one") | ||
| } | ||
| } | ||
|
|
||
| func TestGonumNearestWithMismatchedDimensions(t *testing.T) { | ||
| points := []KDPoint[int]{ | ||
| {ID: "1", Coords: []float64{1, 1}}, | ||
| } | ||
| tree, err := NewKDTree(points, WithBackend(BackendGonum)) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| _, _, ok := tree.Nearest([]float64{1, 1, 1}) | ||
| if ok { | ||
| t.Error("expected no point found, but got one") | ||
| } | ||
| } | ||
|
|
||
| func TestGonumKNearestWithEmptyTree(t *testing.T) { | ||
| _, err := NewKDTree([]KDPoint[int]{}, WithBackend(BackendGonum)) | ||
| if err != ErrEmptyPoints { | ||
| t.Fatalf("expected ErrEmptyPoints, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestGonumRadiusWithEmptyTree(t *testing.T) { | ||
| _, err := NewKDTree([]KDPoint[int]{}, WithBackend(BackendGonum)) | ||
| if err != ErrEmptyPoints { | ||
| t.Fatalf("expected ErrEmptyPoints, got %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating duplicate empty tree tests.
Three separate tests (TestGonumNearestWithEmptyTree, TestGonumKNearestWithEmptyTree, TestGonumRadiusWithEmptyTree) verify the same behaviour: that constructing a KD-tree with an empty point slice returns ErrEmptyPoints. Consider consolidating these into a single test, as the error occurs during construction, not during the query methods.
Comment on lines
+144
to
+150
| func TestGonumNearestWithNilRoot(t *testing.T) { | ||
| backend := &kdBackend{root: nil, dim: 2} | ||
| _, _, ok := gonumNearest[int](backend, []float64{1, 1}) | ||
| if ok { | ||
| t.Error("expected no point found, but got one") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Testing internal implementation details.
This test directly constructs the internal kdBackend struct and calls gonumNearest. Consider using the public API (NewKDTreeFromDim followed by Nearest) to achieve the same test coverage without coupling to internal structures. For example, lines 255-264 demonstrate a similar test using the public API.
🤖 Prompt for AI Agents
In kdtree_gonum_test.go around lines 144 to 150, the test constructs the
internal kdBackend and calls gonumNearest directly, coupling the test to
internal implementation; replace this with the public API by creating a KDTree
with NewKDTreeFromDim(2) (or equivalent constructor) to get an empty tree, then
call tree.Nearest with the query point and assert that no point is found (ok ==
false); ensure the test imports/uses the public types and methods and remove
direct references to kdBackend and gonumNearest so the test verifies behavior
through the public API only.
Comment on lines
+571
to
+593
| func TestGonumKNearestWithMorePoints(t *testing.T) { | ||
| points := []KDPoint[int]{ | ||
| {ID: "1", Coords: []float64{0, 0}}, | ||
| {ID: "2", Coords: []float64{1, 1}}, | ||
| {ID: "3", Coords: []float64{2, 2}}, | ||
| {ID: "4", Coords: []float64{3, 3}}, | ||
| {ID: "5", Coords: []float64{4, 4}}, | ||
| } | ||
| tree, err := NewKDTree(points, WithBackend(BackendGonum)) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| ps, _ := tree.KNearest([]float64{0.5, 0.5}, 3) | ||
| if len(ps) != 3 { | ||
| t.Fatalf("expected 3 points, got %d", len(ps)) | ||
| } | ||
| if !((ps[0].ID == "1" && ps[1].ID == "2") || (ps[0].ID == "2" && ps[1].ID == "1")) { | ||
| t.Errorf("expected first two points to be 1 and 2, got %s and %s", ps[0].ID, ps[1].ID) | ||
| } | ||
| if ps[2].ID != "3" { | ||
| t.Errorf("expected third point to be 3, got %s", ps[2].ID) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify complex assertion logic.
The assertion on lines 587-589 is difficult to read:
if !((ps[0].ID == "1" && ps[1].ID == "2") || (ps[0].ID == "2" && ps[1].ID == "1")) {Consider using a helper function or restructuring the check for clarity:
- if !((ps[0].ID == "1" && ps[1].ID == "2") || (ps[0].ID == "2" && ps[1].ID == "1")) {
- t.Errorf("expected first two points to be 1 and 2, got %s and %s", ps[0].ID, ps[1].ID)
- }
+ ids := []string{ps[0].ID, ps[1].ID}
+ if !((ids[0] == "1" && ids[1] == "2") || (ids[0] == "2" && ids[1] == "1")) {
+ t.Errorf("expected first two points to be 1 and 2 in any order, got %s and %s", ids[0], ids[1])
+ }Or even better, sort and compare:
ids := []string{ps[0].ID, ps[1].ID}
sort.Strings(ids)
if ids[0] != "1" || ids[1] != "2" {
t.Errorf("expected first two points to be 1 and 2 in any order, got %v", ids)
}🤖 Prompt for AI Agents
In kdtree_gonum_test.go around lines 571 to 593, the assertion that the first
two returned IDs are "1" and "2" in any order is written as a nested boolean
expression which is hard to read; replace that check with a clearer approach:
collect the two IDs into a slice, sort the slice, and assert the sorted result
equals ["1","2"] (update the error message to print the sorted ids on failure);
ensure you add the necessary import for sort if not present.
Comment on lines
+211
to
+218
| newPts, _ := Build4DWithStats([]Peer{newPeer}, | ||
| func(p Peer) string { return p.ID }, | ||
| func(p Peer) float64 { return p.Ping }, | ||
| func(p Peer) float64 { return p.Hops }, | ||
| func(p Peer) float64 { return p.Geo }, | ||
| func(p Peer) float64 { return p.Score }, | ||
| weights, invert, stats, | ||
| ) |
There was a problem hiding this comment.
Check the error from Build4DWithStats.
The error return is silently ignored, which could hide failures and lead to panics if newPts is nil or empty when accessed on line 219.
Apply this diff to check the error:
- newPts, _ := Build4DWithStats([]Peer{newPeer},
+ newPts, err := Build4DWithStats([]Peer{newPeer},
func(p Peer) string { return p.ID },
func(p Peer) float64 { return p.Ping },
func(p Peer) float64 { return p.Hops },
func(p Peer) float64 { return p.Geo },
func(p Peer) float64 { return p.Score },
weights, invert, stats,
)
+ if err != nil {
+ t.Fatalf("build new peer err: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| newPts, _ := Build4DWithStats([]Peer{newPeer}, | |
| func(p Peer) string { return p.ID }, | |
| func(p Peer) float64 { return p.Ping }, | |
| func(p Peer) float64 { return p.Hops }, | |
| func(p Peer) float64 { return p.Geo }, | |
| func(p Peer) float64 { return p.Score }, | |
| weights, invert, stats, | |
| ) | |
| newPts, err := Build4DWithStats([]Peer{newPeer}, | |
| func(p Peer) string { return p.ID }, | |
| func(p Peer) float64 { return p.Ping }, | |
| func(p Peer) float64 { return p.Hops }, | |
| func(p Peer) float64 { return p.Geo }, | |
| func(p Peer) float64 { return p.Score }, | |
| weights, invert, stats, | |
| ) | |
| if err != nil { | |
| t.Fatalf("build new peer err: %v", err) | |
| } |
🤖 Prompt for AI Agents
In kdtree_helpers_test.go around lines 211 to 218, the error returned by
Build4DWithStats is being ignored which can mask failures and cause panics when
newPts is used; update the call to capture the error value, check it
immediately, and fail the test if non-nil (e.g., call t.Fatalf or t.Fatal with
the error) so failures are reported and the test stops before dereferencing
newPts.
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.
No description provided.