-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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.