You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/08/19 14:52:26 UTC
[GitHub] [lucene] jtibshirani opened a new pull request #251: LUCENE-10040: Temporarily disable test assertion
jtibshirani opened a new pull request #251:
URL: https://github.com/apache/lucene/pull/251
TestKnnVectorQuery#testDeletes assumes that if there are n total documents, we
can perform a kNN search with k=n and retrieve all documents. This isn't true
with our implementation -- due to randomization we may select less than n entry
points and fail to visit all vectors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jtibshirani commented on pull request #251: LUCENE-10040: Temporarily disable test assertion
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on pull request #251:
URL: https://github.com/apache/lucene/pull/251#issuecomment-904250012
It's indeed a weird case, I'll just relax the assertion but not insist that it will be fixed with layers. I'll make a personal note to revisit it once we have the hierarchical implementation, I think it will fix the behavior but we'll have to test it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on a change in pull request #251: LUCENE-10040: Temporarily disable test assertion
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #251:
URL: https://github.com/apache/lucene/pull/251#discussion_r692192328
##########
File path: lucene/core/src/test/org/apache/lucene/search/TestKnnVectorQuery.java
##########
@@ -350,7 +350,9 @@ public void testDeletes() throws IOException {
toDelete.contains(new Term("index", index)));
allIds.add(index);
}
- assertEquals("search missed some documents", docIndex - toDelete.size(), allIds.size());
+
+ // TODO: reenable this assertion once we implement graph layers
Review comment:
Maybe, in the comment, include the Jira issue for implementing graph layers?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on a change in pull request #251: LUCENE-10040: Temporarily disable test assertion
Posted by GitBox <gi...@apache.org>.
msokolov commented on a change in pull request #251:
URL: https://github.com/apache/lucene/pull/251#discussion_r693039774
##########
File path: lucene/core/src/test/org/apache/lucene/search/TestKnnVectorQuery.java
##########
@@ -350,7 +350,9 @@ public void testDeletes() throws IOException {
toDelete.contains(new Term("index", index)));
allIds.add(index);
}
- assertEquals("search missed some documents", docIndex - toDelete.size(), allIds.size());
+
+ // TODO: reenable this assertion once we implement graph layers
Review comment:
Why does this only happen when we have deletions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on pull request #251: LUCENE-10040: Temporarily disable test assertion
Posted by GitBox <gi...@apache.org>.
msokolov commented on pull request #251:
URL: https://github.com/apache/lucene/pull/251#issuecomment-902778915
OK, so it's a real weird behavior that manifests for very large K or very small N, weird cases. I agree, let's relax the assertion. I am not sure this will be fixed by a multi-level graph though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jtibshirani commented on a change in pull request #251: LUCENE-10040: Temporarily disable test assertion
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on a change in pull request #251:
URL: https://github.com/apache/lucene/pull/251#discussion_r694419360
##########
File path: lucene/core/src/test/org/apache/lucene/search/TestKnnVectorQuery.java
##########
@@ -350,7 +350,9 @@ public void testDeletes() throws IOException {
toDelete.contains(new Term("index", index)));
allIds.add(index);
}
- assertEquals("search missed some documents", docIndex - toDelete.size(), allIds.size());
+
+ // TODO: reenable this assertion once we implement graph layers
Review comment:
In my understanding, this has nothing to do with deletions. I think we could reproduce without deletions, just by using a k equal to the size of the graph.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jtibshirani merged pull request #251: LUCENE-10040: Relax TestKnnVectorQuery#testDeletes assertion
Posted by GitBox <gi...@apache.org>.
jtibshirani merged pull request #251:
URL: https://github.com/apache/lucene/pull/251
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jtibshirani commented on pull request #251: LUCENE-10040: Relax TestKnnVectorQuery#testDeletes assertion
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on pull request #251:
URL: https://github.com/apache/lucene/pull/251#issuecomment-904879940
What timing, our CI just caught another related failure. Here there are no deletions, but we also return fewer than `k` results because `k` is so close to the total number of documents:
```
./gradlew test --tests TestKnnVectorQuery.testRandom -Dtests.seed=3B6CE0E105431E18
```
I opened https://issues.apache.org/jira/browse/LUCENE-10069 so I'm not just tracking in my head!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jtibshirani commented on pull request #251: LUCENE-10040: Temporarily disable test assertion
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on pull request #251:
URL: https://github.com/apache/lucene/pull/251#issuecomment-901987142
I think that if k <= n, the number of documents, then ideally we'd always retrieve k neighbors. I opted to disable the assertion for now instead of making a fix, since we're actively working on graph layers. Adding layers would completely change how entry points are selected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org