Update nearest_neighbours.py for weights=distance#1212
Update nearest_neighbours.py for weights=distance#1212yishiouchen wants to merge 5 commits intoonnx:mainfrom
Conversation
The Mul operator created in line 141 converts distances (node[0]) from OnnxTopK_11 to negative values. The Mul operator is followed by a Max operator, which turns any negative value to 1e-6, so distances lose their effect in the downstream operation. I think this Mul operator should be removed, and node[0] can be connected to the Max operator directly. Signed-off-by: yishiouchen <yi-shiou.chen@3ds.com>
|
Can you look into the broken unit tests? |
|
I fixed the unit tests. Is there any example I could use to complete the PR? |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the nearest-neighbors TopK (opset >= 11) distance output so that weights="distance" uses true (non-negated) distances downstream, instead of collapsing to the 1e-6 clamp.
Changes:
- Remove the post-TopK negation (
Mul(..., -1)) when returning distances fromOnnxTopK_11withkeep_distances=True.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if keep_distances: | ||
| return ( | ||
| node[1], | ||
| OnnxMul(node[0], np.array([-1], dtype=dtype), op_version=op_version), | ||
| node[0] | ||
| ) |
There was a problem hiding this comment.
Changing the opset>=11 keep_distances return from -dist to dist fixes KNN distance-weighting, but it also changes the sign contract of onnx_nearest_neighbors_indices_k for existing callers. In particular, operator_converters/local_outlier_factor.py currently multiplies the returned dist by -1 (to recover positive distances); with this change it will produce negative distances and OnnxMax(..., dist_k) will ignore the real distances. To avoid a regression, make onnx_nearest_neighbors_indices_k(..., keep_distances=True) consistently return positive distances for all opsets (e.g., negate node[0] for opset<11 before returning), and update LocalOutlierFactor’s converter to stop negating the returned distances (or otherwise align it with the new convention).
The Mul operator created in line 141 converts distances (node[0]) from OnnxTopK_11 to negative values. The Mul operator is followed by a Max operator, which turns any negative value to 1e-6, so distances lose their effect in the downstream operation. I think this Mul operator should be removed, and node[0] can be connected to the Max operator directly.