-
Notifications
You must be signed in to change notification settings - Fork 115
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
Change VectorReaderListener to expect number array #416
Conversation
Refactors VectorReaderListener onResponse to expect arrays of Number type from search result instead of Double type. Adds test case to confirm that it can handle Integer type. Cleans up tests in VectorReaderTest class. Signed-off-by: John Mazanec <[email protected]>
|
||
for (int j = 0; j < DEFAULT_DIMENSION; j++) { | ||
vector[j] = random.nextInt(); | ||
} |
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.
nit: you can do the same but with less code via streams:
Integer[] array = random.ints(size, lowBound, highBound).boxed().toArray(Integer[]::new);
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.
Thanks will update
|
||
// Create list of random vectors and ingest | ||
Random random = new Random(); | ||
List<Integer[]> vectors = new ArrayList<>(); |
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.
Is it possible to have mix of lists of different types? If yes I think we need to have such test case with mixed docs.
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.
Oh interesting. Hadn't thought of that. I can add this case.
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.
Before adding the test case can you please validate do we even allow such kind of vectors as input?
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.
Itd be like if someone indexed 2 docs, 1 with [1, 2, 3] and the other with [1.0, 2.0, 3.0]
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.
On second thought, I dont think this would need to be added. Given that we process each hit's vector separately, I dont see the need to add a case for when the hits have different representations.
src/test/java/org/opensearch/knn/training/VectorReaderTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: John Mazanec <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #416 +/- ##
=========================================
Coverage 84.01% 84.01%
Complexity 911 911
=========================================
Files 130 130
Lines 3879 3879
Branches 359 359
=========================================
Hits 3259 3259
Misses 458 458
Partials 162 162
Continue to review full report at Codecov.
|
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-416-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 77353512c1f15e0dc996428a982941a7ee3036fb
# Push it to GitHub
git push --set-upstream origin backport/backport-416-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x Then, create a pull request where the |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.2 1.2
# Navigate to the new working tree
cd .worktrees/backport-1.2
# Create a new branch
git switch --create backport/backport-416-to-1.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 77353512c1f15e0dc996428a982941a7ee3036fb
# Push it to GitHub
git push --set-upstream origin backport/backport-416-to-1.2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.2 Then, create a pull request where the |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-416-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 77353512c1f15e0dc996428a982941a7ee3036fb
# Push it to GitHub
git push --set-upstream origin backport/backport-416-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3 Then, create a pull request where the |
Refactors VectorReaderListener onResponse to expect arrays of Number type from search result instead of Double type. Adds test case to confirm that it can handle Integer type. Cleans up tests in VectorReaderTest class. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 7735351)
…t#416) Refactors VectorReaderListener onResponse to expect arrays of Number type from search result instead of Double type. Adds test case to confirm that it can handle Integer type. Cleans up tests in VectorReaderTest class. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 7735351)
…t#416) Refactors VectorReaderListener onResponse to expect arrays of Number type from search result instead of Double type. Adds test case to confirm that it can handle Integer type. Cleans up tests in VectorReaderTest class. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 7735351)
…t#416) Refactors VectorReaderListener onResponse to expect arrays of Number type from search result instead of Double type. Adds test case to confirm that it can handle Integer type. Cleans up tests in VectorReaderTest class. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 7735351)
…t#416) Refactors VectorReaderListener onResponse to expect arrays of Number type from search result instead of Double type. Adds test case to confirm that it can handle Integer type. Cleans up tests in VectorReaderTest class. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 7735351)
Description
Refactors VectorReaderListener onResponse to expect arrays of Number type from search result instead of Double type. This will allow the training to handle cases where the vectors source is stored as an integer array. Adds test case to confirm that it can handle Integer type. Cleans up tests in VectorReaderTest class.
Credit to @martin-gaievski for the suggested fix.
Issues Resolved
#415
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.