Diversity check bugfix #11781
Diversity check bugfix #11781
Conversation
msokolov
force-pushed
the
diversity-check-bugfix
branch
from
1154045 to
3ba19fd
Compare
msokolov
force-pushed
the
diversity-check-bugfix
branch
from
3ba19fd to
5df91b2
Compare
| {0, 1, 0}, | ||
| {0, 0, 2}, | ||
| {1, 0, 0}, | ||
| {0, 0.4f, 0} |
There was a problem hiding this comment.
hm, I guess this works for bytes too, but we should probably multiply everything here to make it non-fractional
|
@msokolov Thanks for tacking this. I ran ann benchmarks with this change, and happy to confirm that in my test recall with this PR is the same as in 9.3 branch, although QPS is lower, but we can investigate QPSs later. glove-100-angular M:16 efConstruction:100
|
| int candidateIndex, BytesRef candidate, NeighborArray neighbors, float minAcceptedSimilarity) | ||
| throws IOException { | ||
| for (int i = candidateIndex - 1; i > -0; i--) { | ||
| int candidateIndex, BytesRef candidateVector, NeighborArray neighbors) throws IOException { |
There was a problem hiding this comment.
I am surprised that with this big change, we had only a small reduction in recall. I guess the reason could be that in our tests diversity check was really relevant only for small number of nodes; in majority of cases the algorithm just eliminated the most distant node.
There was a problem hiding this comment.
I know - how did this garbage even work at all!
| assertLevel0Neighbors(builder.hnsw, 2, 0); | ||
|
|
||
| builder.addGraphNode(3, vectorsCopy); | ||
| // this is one case we are testing; 2 has been displaced by 3 |
* Fixes bug in HNSW diversity checks introduced in LUCENE-10577
* Fixes bug in HNSW diversity checks introduced in LUCENE-10577
|
It looks like some recent test failures affecting the 9.4 branch are caused by this change, e.g. https://ci-builds.apache.org/job/Lucene/job/Lucene-NightlyTests-9.4/18/ |
|
Apologies, I just noticed that you opened #11787. |
* main: (324 commits) Mute TestKnnVectorQuery#testFilterWithSameScore while we work on a fix Guard FieldExistsQuery against null pointers (apache#11794) Fix handling of ghost fields in string sorts. (apache#11792) LUCENE-10365 Wizard changes contributed from Solr (apache#591) GitHub Workflows security hardening (apache#11789) Improve tessellator performance by delaying calls to the method #isIntersectingPolygon (apache#11786) update DOAP and releaseWizard to reflect migration to github (apache#11747) Diversity check bugfix (apache#11781) Fix rare bug in TestKnnVectorQuery when we have multiple segments GITHUB#11778: Add detailed part-of-speech tag for particle and ending on Nori (apache#11779) LUCENE-10674: Move changes entry to 9.4. apacheGH-11172: remove WindowsDirectory and native subproject. (apache#11774) LUCENE-10674: Ensure BitSetConjDISI returns NO_MORE_DOCS when sub-iterator exhausts. (apache#1068) Removed duplicate check in SpanGradientFormatter (apache#11762) Fix integer overflow in tests. GITHUB#11742: MatchingFacetSetsCounts#getTopChildren now returns top children instead of all children (apache#11764) Retry gradle wrapper download on http 500 and 503. (apache#11766) Fix a typo affecting Luke (apache#11763) Fix IntervalBuilder.NO_INTERVALS docId when unpositioned (apache#11760) LUCENE-10592 Better estimate memory for HNSW graph (apache#11743) ...
I was looking into the changes in recall we have been observing in various test cases. Thanks @jtibshirani for pointing out we should not see any change at all! I found a couple of related bugs in the diversity-checking code that I had introduced when refactoring as part of splitting into float[]- and byte[]- oriented versions. I found some gaps in our test coverage and added tests. One of these demonstrates the bug; the other is just filling in another case.
With this fix I was able to measure the identical recall comparing against test runs with 9.3