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.

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

Enable alternate projection onto space filling curve #646

Conversation

dalg24
Copy link
Contributor

@dalg24 dalg24 commented Mar 4, 2022

Per discussion on #637 enable using 32-bit Morton codes

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.

I am mostly OK with things in ArborX_DetailsTreeConstruction.hpp. But in the BVH interface, I'd rather use enums, unless you want to allow users to provide their own curves (and in that case, I'm not sure what the right operator() interface is).

Have some issues with names, but that can be ironed out later.

src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved
src/ArborX_LinearBVH.hpp Show resolved Hide resolved
src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved
src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved
src/ArborX_LinearBVH.hpp Outdated Show resolved Hide resolved
@aprokop aprokop added the enhancement New feature or request label Mar 4, 2022
@dalg24 dalg24 mentioned this pull request Mar 4, 2022
@dalg24 dalg24 force-pushed the enable_alternate_projection_onto_space_filling_curve branch from 0ce69da to abc0646 Compare March 6, 2022 01:14
examples/viz/tree_visualization.cpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsBatchedQueries.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsBatchedQueries.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsBatchedQueries.hpp Show resolved Hide resolved
src/details/ArborX_DetailsTreeConstruction.hpp Outdated Show resolved Hide resolved

return LLONG_MIN + (i ^ (i + 1));
return x + (!x) * (min_value + (i ^ (i + 1))) - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

You accidentally introduced a subtlety here that one need to remember about. It is still correct, but now also depends on the fact that i ^ (i+1) > 0. Otherwise, you would have subtracted -1 from min_value.

Actually, I'm starting to dislike the whole x + !x thing. I am not sure it benefits the performance anymore, and it looks complex. We could try to

     auto const x = _sorted_morton_codes(i) ^ _sorted_morton_codes(i + 1);
     if (x)
       return x - 1;
     return min_value + i ^ (i + 1);

Does not have to be a part of this PR, though.

@dalg24 dalg24 force-pushed the enable_alternate_projection_onto_space_filling_curve branch from 85caef5 to 68c0b28 Compare March 7, 2022 03:16
@aprokop aprokop added this to the Tentative 1.2 Release milestone Mar 7, 2022
@dalg24 dalg24 force-pushed the enable_alternate_projection_onto_space_filling_curve branch from 68c0b28 to 97692e6 Compare March 7, 2022 22:21
@aprokop
Copy link
Contributor

aprokop commented Mar 8, 2022

I did a quick performance check on HACC using both DBSCAN and MST. The timings are identical to master, no performance regression.

@dalg24 dalg24 merged commit e81514a into arborx:master Mar 8, 2022
@dalg24 dalg24 deleted the enable_alternate_projection_onto_space_filling_curve branch March 8, 2022 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants