[MOD-14955] Add DistanceCalculatorWithNorm - #1002
Conversation
f438a59 to
d37077d
Compare
4deae2b to
a0be70d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1002 +/- ##
==========================================
- Coverage 97.16% 97.15% -0.02%
==========================================
Files 141 141
Lines 8293 8328 +35
==========================================
+ Hits 8058 8091 +33
- Misses 235 237 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d37077d to
7eaa73f
Compare
c559e31 to
5070acb
Compare
5070acb to
7a8360f
Compare
c996408 to
b9023e4
Compare
| m1 += mean[i + 1] * y1; | ||
| m2 += mean[i + 2] * y2; | ||
| m3 += mean[i + 3] * y3; | ||
| m0 += mean[i + 0] * to_fp32<DataType>(original_input[i + 0]); |
There was a problem hiding this comment.
P3: Centered L2 no longer consumes x_mean_ip, y_mean_ip, or mean_sum_squares, but WithNorm still computes both dot products and stores an extra float in every L2 storage/query blob. Make correction metadata conditional on Metric == VecSimMetric_IP to avoid dead O(dim) preprocessing work and storage.
There was a problem hiding this comment.
Fixed in e0a0a20: gated the extra mean_ip metadata slot/computation to WithNorm && Metric == VecSimMetric_IP (in sq8.h and preprocessors.h) instead of WithNorm alone. L2 no longer computes or stores x_mean_ip/y_mean_ip since the centered-L2 correction never reads them. Also had to fix a pre-existing test in QuantPreprocessorWithNormMetricTest (1b39f3f) that was asserting on mean_ip_index<Metric>() unconditionally for L2 too — caught by Bugbot after the metadata layout changed size.
| } | ||
| } | ||
|
|
||
| TEST(DistanceCalculatorWithNormTest, RandomVectors) { |
There was a problem hiding this comment.
P4: The underlying odd-dimension SQ8 kernels are covered by #1001, but the promised odd-dimension DistanceCalculatorWithNorm integration case is still missing. Add one to exercise the complete normalized metadata layout and correction path.
There was a problem hiding this comment.
Added DistanceCalculatorWithNormTest.OddDimension_MatchesBruteForce (dim=13) covering IP and L2 symmetric + asymmetric paths through the unaligned storage-blob metadata offset.
ofiryanai
left a comment
There was a problem hiding this comment.
Blocking correctness and alignment concerns are resolved. The remaining P3/P4 comments are non-blocking.
…test L2's normalized correction cancels mean terms exactly during centering and never reads mean_ip (see DistanceCalculatorWithNorm), so gate the extra mean_ip metadata slot/computation to IP only instead of any WithNorm index. Also add an odd-dimension (dim=13) integration test for DistanceCalculatorWithNorm covering IP/L2 symmetric+asymmetric paths, exercising the unaligned metadata offset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e0a0a20. Configure here.
sq8::mean_ip_index/query_mean_ip_index now resolve to the sum_squares slot for L2 since that metric no longer carries a mean_ip metadata entry. Guard the mean_ip assertions to IP only so the L2 case doesn't compare unrelated metadata slots. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI showed absolute errors up to ~0.57 for these larger-magnitude fixture vectors (0.05f was tuned for the smaller dim=8 fixtures elsewhere in the file). The test's purpose is to catch alignment/ indexing bugs in the odd-dim metadata layout, not to bound SQ8 quantization precision, so 1.0f still catches gross corruption while tolerating normal quantization error at this magnitude. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Describe the changes in the pull request
This PR is stacked on #1000. Review only the incremental diff against
dor-forer-MOD-14952-support-quant-normalization.Cherry-picks the two commits from ARM-software/VectorSimilarity-for-Arm#3:
493ee7a8andc27401d6.This PR adds
DistanceCalculatorWithNorm, which invokes the selected SQ8 kernel and applies the mean-normalization correction terms needed to recover the distance between the original vectors. The follow-up commit makes metadata reads safe for unaligned storage.The calculator integrates with the construction-time dispatch introduced in #999:
mean_sum_squares.IndexCalculatorInterface::calcDistanceorcalcDistanceForQuery, so it does not restore the per-distance vtable lookup removed by MOD-14916 Devirtualize HNSW / brute-force search hot path #937.Tests cover FP32/FP16, L2/IP, symmetric/asymmetric, zero-mean, randomized-vector, and stateless/stateful dispatch behavior.
Which issues this PR fixes
Main objects this PR modified
DistanceCalculatorWithNormMark if applicable
Validation
make formatmake check-formattest_componentsandtest_hnswtest_components: 40/40 passedtest_hnswexcluding the two external serialization fixtures: 169/169 passedgit diff --check dor-forer-MOD-14952-support-quant-normalization...HEADDistanceDispatchaveraged 2.69 ns/call (no measurable regression within run-to-run noise)Note
Medium Risk
Changes core VecSim distance math and blob layouts for normalized SQ8 indices; mistakes would skew ANN search, but behavior is heavily covered by new unit tests including numerical edge cases.
Overview
Adds
DistanceCalculatorWithNorm, which runs the existing SQ8 distance kernels on mean-centered quantized storage and applies IP-only correction terms (x_mean_ip,y_mean_ip,mean_sum_squares) so reported distances match the original (unshifted) vectors. Normalized L2 skips runtime correction: queries are centered at preprocess time (y - mean), so stored/query kernels already compute the true L2 distance; L2 paths expose statelessDistanceDispatch(raw kernels), while IP uses stateful dispatch for the correction callbacks.QuantPreprocessor(WithNorm) is aligned with that split: L2 query blobs store centered values and metadata sums over centered data; IP keeps raw query values and recordsy_mean_ipfrom the original query.sq8metadata layout only adds the extramean_ipslot when WithNorm + IP (not for normalized L2).Unit tests cover FP32/FP16, L2/IP, symmetric/asymmetric paths, zero mean, large-offset L2 stability, random vectors, odd dimensions (unaligned metadata reads via
load_unaligned), and dispatch stateless vs stateful behavior.Reviewed by Cursor Bugbot for commit 385741f. Bugbot is set up for automated code reviews on this repo. Configure here.