Skip to content

[MOD-14955] Add DistanceCalculatorWithNorm - #1002

Merged
dor-forer merged 9 commits into
mainfrom
dor-forer-MOD-14955-distance-calculator-with-norm
Jul 29, 2026
Merged

[MOD-14955] Add DistanceCalculatorWithNorm#1002
dor-forer merged 9 commits into
mainfrom
dor-forer-MOD-14955-distance-calculator-with-norm

Conversation

@dor-forer

@dor-forer dor-forer commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

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: 493ee7a8 and c27401d6.

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:

  • HNSW caches a non-virtual callback and a minimal immutable context containing only the stored/query kernels and mean_sum_squares.
  • The candidate loop does not call IndexCalculatorInterface::calcDistance or calcDistanceForQuery, so it does not restore the per-distance vtable lookup removed by MOD-14916 Devirtualize HNSW / brute-force search hot path #937.
  • Existing calculators retain the stateless raw-kernel path. Stored-to-stored normalized L2 also uses that path because its mean terms cancel.

Tests cover FP32/FP16, L2/IP, symmetric/asymmetric, zero-mean, randomized-vector, and stateless/stateful dispatch behavior.

Which issues this PR fixes

  1. MOD-14955

Main objects this PR modified

  1. DistanceCalculatorWithNorm
  2. Normalized-distance component unit tests

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Validation

  • make format
  • make check-format
  • Release build of test_components and test_hnsw
  • test_components: 40/40 passed
  • test_hnsw excluding the two external serialization fixtures: 169/169 passed
  • git diff --check dor-forer-MOD-14952-support-quant-normalization...HEAD
  • Focused stateless-dispatch microbenchmark: 36 alternating-order rounds, 20M calls per path per round; cached raw kernel averaged 2.75 ns/call and DistanceDispatch averaged 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 stateless DistanceDispatch (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 records y_mean_ip from the original query. sq8 metadata layout only adds the extra mean_ip slot 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.

Comment thread src/VecSim/vec_sim_index.h
@dor-forer
dor-forer force-pushed the dor-forer-MOD-14952-support-quant-normalization branch from f438a59 to d37077d Compare July 23, 2026 11:59
@dor-forer
dor-forer force-pushed the dor-forer-MOD-14955-distance-calculator-with-norm branch from 4deae2b to a0be70d Compare July 23, 2026 12:14
@dor-forer
dor-forer marked this pull request as ready for review July 23, 2026 12:21
@dor-forer
dor-forer requested a review from ofiryanai July 23, 2026 12:22
Comment thread src/VecSim/spaces/computer/calculator.h Outdated
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.15%. Comparing base (de00dd5) to head (385741f).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dor-forer
dor-forer force-pushed the dor-forer-MOD-14952-support-quant-normalization branch from d37077d to 7eaa73f Compare July 26, 2026 06:33
@dor-forer
dor-forer force-pushed the dor-forer-MOD-14955-distance-calculator-with-norm branch from c559e31 to 5070acb Compare July 26, 2026 07:38
Base automatically changed from dor-forer-MOD-14952-support-quant-normalization to main July 26, 2026 14:22
@dor-forer
dor-forer force-pushed the dor-forer-MOD-14955-distance-calculator-with-norm branch from 5070acb to 7a8360f Compare July 27, 2026 08:27
Comment thread src/VecSim/spaces/computer/calculator.h Outdated
Comment thread src/VecSim/spaces/computer/calculator.h
@dor-forer
dor-forer requested a review from ofiryanai July 27, 2026 14:00
@dor-forer
dor-forer force-pushed the dor-forer-MOD-14955-distance-calculator-with-norm branch from c996408 to b9023e4 Compare July 28, 2026 15:20
@dor-forer
dor-forer enabled auto-merge July 29, 2026 08:40
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]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added DistanceCalculatorWithNormTest.OddDimension_MatchesBruteForce (dim=13) covering IP and L2 symmetric + asymmetric paths through the unaligned storage-blob metadata offset.

ofiryanai
ofiryanai previously approved these changes Jul 29, 2026

@ofiryanai ofiryanai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Blocking correctness and alignment concerns are resolved. The remaining P3/P4 comments are non-blocking.

@dor-forer
dor-forer added this pull request to the merge queue Jul 29, 2026
@dor-forer
dor-forer removed this pull request from the merge queue due to a manual request Jul 29, 2026
…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>

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread tests/unit/test_components.cpp
dor-forer and others added 3 commits July 29, 2026 14:11
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>
@dor-forer
dor-forer requested a review from ofiryanai July 29, 2026 12:46
@dor-forer
dor-forer added this pull request to the merge queue Jul 29, 2026
Merged via the queue into main with commit efd63da Jul 29, 2026
16 checks passed
@dor-forer
dor-forer deleted the dor-forer-MOD-14955-distance-calculator-with-norm branch July 29, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants