Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable early termination of the tree traversal for spatial predicates #427

Merged
merged 11 commits into from
Dec 7, 2020
Merged

Enable early termination of the tree traversal for spatial predicates #427

merged 11 commits into from
Dec 7, 2020

Conversation

dalg24
Copy link
Contributor

@dalg24 dalg24 commented Dec 6, 2020

Implement #407

@dalg24 dalg24 changed the title Let callback return a hint for early termination of the tree traversal for spatial predicates Enable early termination of the tree traversal for spatial predicates Dec 6, 2020
aprokop
aprokop requested changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things that should be done:

  • enforce either enum or void as return type
  • prohibit using with BufferOptimization

test/tstCallbacks.cpp Show resolved Hide resolved
dalg24
dalg24 commented Dec 7, 2020
View reviewed changes
src/details/ArborX_Callbacks.hpp
@@ -20,6 +20,13 @@

namespace ArborX
{

enum class CallbackTreeTraversalControl
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any opinion on the name of the enum? It might be less relevant with the BruteForce data structure or a binning strategy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change it to CallbackControl or similar, making it more general than just a tree.

Copy link
Contributor Author

@dalg24 dalg24 Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it because it does not indicate what it controls and could even mislead into thinking controls the callback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not going to bikeshed, it's acceptable.

dalg24
dalg24 commented Dec 7, 2020
View reviewed changes
test/tstCallbacks.cpp Outdated Show resolved Hide resolved
@dalg24 dalg24 force-pushed the early_exit branch 2 times, most recently from 08c8d96 to 3cfa8d5 Compare December 7, 2020 20:01
aprokop
aprokop reviewed Dec 7, 2020
View reviewed changes
test/tstCallbacks.cpp Outdated Show resolved Hide resolved
@dalg24
Invoke callback and check early exit in tree traversals
144e937 
In TreeTraversal specialization for spatial predicates.
@dalg24
Add archetypal for user callbacks in recently added query() overload
ed8f654 
Callbacks that do not take an output functor as trailing argument.
@dalg24
Define strong enumeration to let callback signify early exit or norma…
3c0bdbc 
…l continuation"
@dalg24
Define function that invoke the callback and check hint for early exit
d110d98
@dalg24
Forward return value of user callback taking output functor trailing arg
fa917ac
@dalg24
Add unit test for callback returning early exit hint
a8aa6b6
@dalg24
Move check for generic lambda with nvcc to a function
cb01a39
@dalg24
Check signature of callbacks in recently added query() overload
9b71461
@dalg24
Add unit tests for callbacks validation
97cc94b
@dalg24
Require callback return type to be void or CallbackTreeTraversalControl
1c56a42
@dalg24
Improve static assertion message about callback return type
e5b5b67
@dalg24
Copy link
Contributor Author

dalg24 commented Dec 7, 2020

Rebased and all builds are passing

@aprokop aprokop merged commit 74cf90f into arborx:master Dec 7, 2020
@aprokop aprokop mentioned this pull request Dec 7, 2020
Merged
@dalg24 dalg24 deleted the early_exit branch December 8, 2020 00:17
This was referenced Dec 8, 2020
Closed
Closed
@aprokop aprokop added enhancement New feature or request performance Something is slower than it should be labels Dec 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aprokop aprokop aprokop approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request performance Something is slower than it should be
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dalg24 @aprokop