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

Jump to bottom

Allow different volumes for leaf and internal nodes #897

Merged
merged 1 commit into from
Jun 16, 2023
Merged

Allow different volumes for leaf and internal nodes #897

merged 1 commit into from
Jun 16, 2023

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Jun 15, 2023

No description provided.

@aprokop aprokop mentioned this pull request Jun 15, 2023
Open
@aprokop aprokop force-pushed the 9-allow_different_volumes branch from c4ea987 to c6b6ccd Compare June 15, 2023 15:26
@aprokop aprokop changed the title Allow different types for leaf and internal nodes Allow different volumes for leaf and internal nodes Jun 15, 2023
@aprokop
Copy link
Contributor Author

aprokop commented Jun 15, 2023 •
edited
Loading

For the most part, the results from Ascent benchmarks are fine. Some improvement in Serial queries; the only slowdown is kNN callback (5-8%).

Cuda has about 5% slowdown in radius search for dense geometries. This is expected given that there's now more divergence.

@aprokop aprokop added the performance Something is slower than it should be label Jun 15, 2023
dalg24
dalg24 reviewed Jun 16, 2023
View reviewed changes
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

How do we know these are all the changes that were required? Is there any test that can be added at this time (runtime and/or compile-time)?

@aprokop
Copy link
Contributor Author

aprokop commented Jun 16, 2023 •
edited
Loading

How do we know these are all the changes that were required? Is there any test that can be added at this time (runtime and/or compile-time)?

These are not all the changes that are required. Callbacks will have to be wrapped, too. I have a DBSCAN change that allows to use PairIndexVolume<Point> for half-traversal that is running. However, beyond this patch, it requires at least few others (e.g., this).

I decided to create this PR to make this change independent of others, and to easier track the performance regresson. It can be done in parallel with the existing PRs (#892), and will have to be done anyway.

dalg24
dalg24 approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Fine

@aprokop aprokop merged commit 0845bdc into arborx:master Jun 16, 2023
@aprokop aprokop deleted the 9-allow_different_volumes branch June 16, 2023 15:02
aprokop added a commit to aprokop/ArborX that referenced this pull request Dec 7, 2023
@aprokop
Change spatial traversal to fix perf regression from arborx#897
c61695a
@dalg24 dalg24 mentioned this pull request Dec 7, 2023
Merged
aprokop added a commit that referenced this pull request Dec 7, 2023
@aprokop
Merge pull request #976 from aprokop/fixing_perf
851e915 
Change spatial traversal to fix perf regression from #897
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@dalg24 dalg24 dalg24 approved these changes

Assignees
No one assigned
Labels
performance Something is slower than it should be
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aprokop @dalg24