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

Switch to using expand for scene reductions #1082

Merged
merged 5 commits into from
May 13, 2024
Merged

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented May 4, 2024

Right now, we do a combination of += and expand in our reduction. I would like to switch to only using expand in order to make the geometric algorithms a general as possible. += relies on internal implementation, while expand() tells us exactly what operations a, say, Box geometry is required to provide.

The goal here is to move all stuff from inside the geometry classes (this includes KDOP and Ray, too) into generic algorithms.

  • Deprecate reduction_identity in ArborX::Box and ArborX::ExperimentalHyperGeometry::Box (not sure why we didn't have one in KDOP)
  • Deprecate += in ArborX::Box
  • Remove += in ArborX::ExperimentalHyperGeometry::Box
  • Switch scene calculation to using functors with join operation using expand
  • Provide a generic implementation of expand(Box, Point) and expand(Box, Box)

Note that I didn't have to change tstCompileOnlyTypeRequirement.cpp test.

Next step would be redoing KDOP geometry in #982 to include dimension and coordinate, and reorganize it so that there are no internal class functions.

@aprokop aprokop force-pushed the no_sum branch 2 times, most recently from c4ab80f to 4416b76 Compare May 4, 2024 18:39
Copy link
Collaborator

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

I'm in favor of avoiding using operator+= and Kokkos::Sum.

src/details/ArborX_DetailsBruteForceImpl.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsTreeConstruction.hpp Outdated Show resolved Hide resolved
test/tstIndexableGetter.cpp Outdated Show resolved Hide resolved
Co-authored-by: Daniel Arndt <arndtd@ornl.com>
@aprokop
Copy link
Contributor Author

aprokop commented May 13, 2024

CUDA builds did not start (failed to check out the source). SYCL timed out. That's ok. Merging.

@aprokop aprokop merged commit 1270d7f into arborx:master May 13, 2024
1 of 2 checks passed
@aprokop aprokop deleted the no_sum branch May 13, 2024 21:39
@aprokop aprokop mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants