-
Notifications
You must be signed in to change notification settings - Fork 33
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
Switch top tree in DistributedTree to new API and do some cleanup #1015
Conversation
ce945b7
to
3d62a02
Compare
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.
Looks OK.
3d62a02
to
f386c4c
Compare
What changed? |
Sorry, I should have made a separate commit. I included few extra headers as the self-containment tests were failing. |
DistributedTree const &tree, Indices &indices, | ||
Offset &offset, Distances &); | ||
DistributedTree const &tree, | ||
Predicates const &queries, Distances const &, |
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.
Why did you do change
(indices, offsets, distances) -> (distances, indices, offsets)
and why did you const the distances?
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.
We only read the distances, and don't modify them. So const
makes it clearer.
I changed the order to follow the same principle: input arguments followed by output arguments.
aca1a96
to
b7bf542
Compare
Rebased on #1016. |
Distances
args inreassesStrategy
anddeviseStrategy
to constreassesStrategy
anddeviseStrategy
to align with queryBoundingVolume
alias inDistributedTree
indices
tonearest_ranks
andintersected_ranks
in DistributedTree queries implementation