-
Notifications
You must be signed in to change notification settings - Fork 144
harden tests for heap graph reconstruction #543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
harden tests for heap graph reconstruction #543
Conversation
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
e3fd3be to
ae4120c
Compare
fix minor bug in construction Signed-off-by: Samuel Herman <[email protected]>
ae4120c to
98507a5
Compare
Signed-off-by: Samuel Herman <[email protected]>
Signed-off-by: Samuel Herman <[email protected]>
Signed-off-by: Samuel Herman <[email protected]>
Signed-off-by: Samuel Herman <[email protected]>
Signed-off-by: Samuel Herman <[email protected]>
|
There seems to be a failure from one of the tests that this PR is adding. In |
…l calculation Signed-off-by: Samuel Herman <[email protected]>
Signed-off-by: Samuel Herman <[email protected]>
Signed-off-by: Samuel Herman <[email protected]>
Yeah, it is now more robust, the recall calculation need to be more statistically significant so I increased number of vectors + beamsize and number of query vectors participating in average recall calculation. |
marianotepper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments here and there.
jvector-base/src/main/java/io/github/jbellis/jvector/graph/GraphIndexBuilder.java
Outdated
Show resolved
Hide resolved
jvector-base/src/main/java/io/github/jbellis/jvector/graph/GraphIndexBuilder.java
Show resolved
Hide resolved
jvector-tests/src/test/java/io/github/jbellis/jvector/graph/OnHeapGraphIndexTest.java
Show resolved
Hide resolved
jvector-tests/src/test/java/io/github/jbellis/jvector/graph/OnHeapGraphIndexTest.java
Show resolved
Hide resolved
jvector-tests/src/test/java/io/github/jbellis/jvector/graph/OnHeapGraphIndexTest.java
Show resolved
Hide resolved
Signed-off-by: Samuel Herman <[email protected]>
|
One of the tests failed due to a different test not related to this change: I am re-running this test so we can have a whole green panel |
marianotepper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
jshook
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice to see added docs
Description
buildAndMergemethod -important for downstream dependencies with centralized resource management