Fix recall computation for fewer than k groundtruth results

[magdalendobson]

Groundtruth for filtered datasets uses the .rangeres format, since a filtered query may have few or no results. However, at the time of recall computation for top-k search, fewer than k results causes an error. This PR patches that error and computes recall for each point using the following paradigm:

1. If the point has 0 < k' <= k groundtruth results, recall is the number of correctly reported points divided by k'.
2. If the point has no groundtruth results, it is completely excluded from the recall calculation.

The average recall is then the sum of all recalls divided by the number of non-zero results. The points with zero results do not affect recall. The reasoning for this is that in a dataset with majority-null results for filtered queries, we want recall to capture our performance on the queries that actually have results, rather than being artificially high when we always correctly report that these points have no results.

Since this error will no longer trigger even for a regular top-k search in recall computation, I have pushed the error for not enough groundtruth results to when the groundtruth in the regular bin format is actually read. This is an improvement since a failure will now trigger before an index build and search, making a better experience for the user. I leave some of the instances of NotEnoughGroundTruth inside recall computation as an extra layer of failsafe, but they could also be removed if reviewers feel we should completely remove them.

Along the way I get rid of the minimum and maximum values in the recall stats, since after some discussion with the team it appears that no one uses them.

[magdalendobson]

@microsoft-github-policy-service agree company="Microsoft"

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes recall computation when filtered/variable-length groundtruth contains fewer than k results by computing per-query recall against the available groundtruth and excluding zero-result queries from the average.

Changes:

• Update recall computation to handle per-row k' <= k groundtruth sizes and ignore empty groundtruth rows in the average.
• Move “insufficient groundtruth for top-k” validation to groundtruth loading for fixed-size .bin groundtruth.
• Remove minimum / maximum from reported recall metrics.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File	Description
diskann-benchmark/src/utils/recall.rs	Removes min/max fields from the benchmark-facing recall metrics wrapper.
diskann-benchmark/src/utils/datafiles.rs	Adds optional k validation when loading groundtruth from .bin.
diskann-benchmark/src/backend/index/benchmarks.rs	Computes max recall_k across runs and passes it into groundtruth loading.
diskann-benchmark-core/src/recall.rs	Implements variable-length groundtruth recall + removes min/max metrics and updates tests accordingly.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.84127% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.46%. Comparing base (895a2c0) to head (e3cc3f4).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
diskann-benchmark/src/backend/index/benchmarks.rs	30.00%	7 Missing ⚠️
diskann-benchmark/src/backend/index/search/knn.rs	50.00%	5 Missing ⚠️
diskann-benchmark/src/utils/datafiles.rs	96.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1069      +/-   ##
==========================================
+ Coverage   89.45%   89.46%   +0.01%     
==========================================
  Files         458      458              
  Lines       85398    85530     +132     
==========================================
+ Hits        76392    76520     +128     
- Misses       9006     9010       +4     

Flag	Coverage Δ
miri	89.46% <94.84%> (+0.01%)	⬆️
unittests	89.09% <94.84%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
diskann-benchmark-core/src/recall.rs	98.50% <100.00%> (+0.70%)	⬆️
diskann-benchmark-core/src/search/graph/knn.rs	95.08% <100.00%> (+0.16%)	⬆️
...iskann-benchmark-core/src/search/graph/multihop.rs	98.72% <100.00%> (+0.03%)	⬆️
diskann-benchmark/src/backend/exhaustive/minmax.rs	100.00% <ø> (ø)
...iskann-benchmark/src/backend/exhaustive/product.rs	100.00% <ø> (ø)
...kann-benchmark/src/backend/exhaustive/spherical.rs	100.00% <ø> (ø)
diskann-benchmark/src/backend/index/spherical.rs	100.00% <ø> (ø)
diskann-benchmark/src/inputs/graph_index.rs	36.28% <100.00%> (+0.32%)	⬆️
diskann-benchmark/src/utils/recall.rs	57.14% <ø> (-5.36%)	⬇️
diskann-benchmark/src/utils/datafiles.rs	87.36% <96.55%> (+3.78%)	⬆️
... and 2 more

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[magdalendobson]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved in 934789c. The merge conflict in diskann-benchmark/src/inputs/graph_index.rs has been resolved by:

• Keeping the max_k() method from the feature branch (needed for the recall computation fix)
• Adopting the new validate() method pattern from main (replaces the CheckDeserialization trait)
• Updating method calls from check_deserialization to resolve

All tests pass and clippy/fmt checks are clean.

[hildebrandmw]

I had a thought - should this be behind a configuration option? (maybe ditching allow_insufficient_results since I think that should just always be the case).

My concern is that if we do a normal k-nearest-neighbors search and request recall at 100, but the groundtruth only has 10 nearest neighbors, we will silently get recall computed for 10 nearest neighbors instead of the expected 100. Basically, we need a way to opt-out of this behavior.

Here is my proposal:

1. We ditch the allow_insufficient_results flag. Algorithms can't always ensure this and it feels like something that callers should enforce anyways.

2. Add the following enum as an argument:

   pub enum GroundTruthMode {
       Fixed,
       Flexible,
   }

   In Fixed mode, we ensure we have at least recall_k entries, and Flexible mode allows what's happening in this PR.

   Using an enum makes updating the callsites easier and more explicit: we aren't changing one bool for another.

3. Update diskann_benchmark::utils::compute_multiple_recalls to do one upfront check that the results have at least recall_n entries.

4. Update the KNN searches to use GroundTruthMode::Fixed and the filtered KNN searches to use GroundTruthMode::Flexible

[magdalendobson]

I had a thought - should this be behind a configuration option? (maybe ditching allow_insufficient_results since I think that should just always be the case).

My concern is that if we do a normal k-nearest-neighbors search and request recall at 100, but the groundtruth only has 10 nearest neighbors, we will silently get recall computed for 10 nearest neighbors instead of the expected 100. Basically, we need a way to opt-out of this behavior.

Here is my proposal:

1. We ditch the allow_insufficient_results flag. Algorithms can't always ensure this and it feels like something that callers should enforce anyways.

2. Add the following enum as an argument:

   pub enum GroundTruthMode {
       Fixed,
       Flexible,
   }

   In Fixed mode, we ensure we have at least recall_k entries, and Flexible mode allows what's happening in this PR.
   Using an enum makes updating the callsites easier and more explicit: we aren't changing one bool for another.

3. Update diskann_benchmark::utils::compute_multiple_recalls to do one upfront check that the results have at least recall_n entries.

4. Update the KNN searches to use GroundTruthMode::Fixed and the filtered KNN searches to use GroundTruthMode::Flexible

I believe this is resolved now

[hildebrandmw]

Thanks Magdalen - two small comments then looks good to me!