-
Notifications
You must be signed in to change notification settings - Fork 41
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
Implement APIv2 BruteForce #962
Conversation
2ef333e
to
88401ad
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.
Did you look at performance?
template <class ExecutionSpace, class Primitives, class BoundingVolumes, | ||
class Bounds> | ||
template <class ExecutionSpace, class Values, class IndexableGetter, | ||
class Nodes, class Bounds> |
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.
I am not a big fan of that name "Nodes" but I understand we need to distinguish form the user-provided input and I don't have a better idea at that time.
A100 results (OACISS/Saturn),
This is definitely a consequence of both storing less data for the benchmark (points instead of boxes) as well as the faster intersection routine (point/sphere vs box/sphere). |
template <typename ExecutionSpace, typename Predicates, typename Callback, | ||
typename Ignore = int> | ||
void query(ExecutionSpace const &space, Predicates const &predicates, | ||
Callback const &callback, Ignore = Ignore()) 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.
I would just omit the last template parameter for now or if we want to have compatibility with BasicBoundingVolumeHierarchy
actually allow passing in TraversalPolicy
but enforce it being default-constructed.
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.
I'd be ok to do that in a different PR. Though, there is a question of backwards compatibility if one just removes the last argument.
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.
Fine with me.
"template <class MemorySpace>\n" | ||
"using ArborX_BruteForce_Box = ArborX::BruteForce<MemorySpace, ArborX::Box>;\n" | ||
"template <class MemorySpace>\n" | ||
"using ArborX_Legacy_BasicBruteForce_Box =\n" |
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.
As I commented on the BVH PR I don't like too much the alias name.
I would prefer ArborX_Legacy_BruteForce[_Box]
All builds that started finished correctly. Some builds did not start ( |
Change the BruteForce interface to follow the changes in BVH.