Skip to content

Fix KDTree Edge pruning + optimize traversal - #422

Open
andrewdalpino wants to merge 6 commits into
3.0from
fix-kdtree-edge-case-pruning
Open

Fix KDTree Edge pruning + optimize traversal#422
andrewdalpino wants to merge 6 commits into
3.0from
fix-kdtree-edge-case-pruning

Conversation

@andrewdalpino

Copy link
Copy Markdown
Member

KDTree nearest()/range() can return wrong neighbors — src/Graph/Trees/KDTree.php:202-211, 267-278, src/Graph/Nodes/Box.php:148-152 Pruning tests use corner distances (Box::sides() yields only min/max corners). A corner distance is neither a valid lower nor upper bound for points inside the box, so valid neighbors get pruned. Reproduced: nearest() missed true neighbors in ~5/200 trials; range() returned 0 points where brute force found 4–7. Affects KNN, RadiusNeighbors, KDNeighbors, DBSCAN, MeanShift when KDTree is selected (BallTree/VantageTree are correct).

@andrewdalpino
andrewdalpino requested review from a team and a lite review from Copilot August 15, 2026 23:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes incorrect pruning behavior in KDTree::nearest() and KDTree::range() by replacing corner-distance checks with a lower-bound distance from the query point to a node’s bounding box, preventing valid neighbors from being pruned during traversal. It also updates/extends the KDTree test suite and records the change in the changelog.

Changes:

  • Replace corner-distance pruning with bounding-box lower-bound pruning via KDTree::minDistance().
  • Add brute-force regression tests to ensure nearest() and range() match expected results on small deterministic datasets.
  • Update changelog entry for the new patch release.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/Graph/Trees/KDTree.php Fixes traversal pruning logic by computing a bounding-box distance lower bound.
tests/Graph/Trees/KDTreeTest.php Updates expected range counts and adds brute-force regression tests for nearest()/range().
tests/Regressors/KDNeighborsRegressorTest.php Adjusts test expectations for KDNeighborsRegressor params/score threshold.
CHANGELOG.md Adds a 2.5.3 entry documenting the KDTree pruning fix.
Suppressed comments (1)

tests/Regressors/KDNeighborsRegressorTest.php:139

  • The test expects params()['k'] to be 4, but the estimator is constructed with k=5 in setUp(). This will fail and also makes the test inconsistent with the configured hyperparameter.
    {
        $expected = [
            'k' => 4,
            'weighted' => true,
            'tree' => new KDTree(),
        ];


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Graph/Trees/KDTree.php Outdated
@andrewdalpino
andrewdalpino changed the base branch from master to 3.0 August 15, 2026 23:50
@andrewdalpino
andrewdalpino requested a lite review from Copilot August 16, 2026 00:12
@andrewdalpino andrewdalpino added the optimization Make something perform faster label Aug 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (4)

tests/Regressors/KDNeighborsRegressorTest.php:38

  • MIN_SCORE was changed to an untyped class constant, but other estimator tests use typed float constants for MIN_SCORE. Keeping the float type here maintains consistency and prevents accidental non-float assignments in the future.
     * The minimum validation score required to pass the test.
     */
    protected const MIN_SCORE = 0.89;

src/Graph/Trees/KDTree.php:81

  • The new BoxPrunable kernel check uses the and operator, which has surprising precedence in PHP and is easy to misread. Using an explicit null check with && avoids precedence pitfalls and matches common project style.
        if ($kernel and !$kernel instanceof BoxPrunable) {
            throw new InvalidArgumentException('Distance kernel must implement the'
                . ' BoxPrunable interface.');

CHANGELOG.md:14

  • This change introduces a new public constraint (KDTree now requires distance kernels to implement BoxPrunable). It would be helpful to call this out explicitly in the changelog entry since it can affect downstream users providing custom kernels.
    - Fix KDTree edge pruning + optimize traversal

src/Kernels/Distance/BoxPrunable.php:7

  • BoxPrunable is used as a marker interface to control KDTree pruning behavior, but the docblock doesn't describe what property implementers are asserting. Adding a brief contract statement here will make it clearer for anyone implementing custom distance kernels.
 * Box Prunable
 *

@andrewdalpino andrewdalpino added the bug Something isn't working label Aug 16, 2026

@apphp apphp left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewdalpino please resolve merge conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working optimization Make something perform faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants