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

feat: Implement a k-d tree data structure #987

Merged
merged 1 commit into from
Sep 14, 2021

Conversation

stephenswat
Copy link
Member

This pull request is part three of four in my attempt to implement an orthogonal range search seed finder. Parts one and two are #985 and #986.

k-d trees are data structures that allow certain operations to be performed efficiently on point cloud data. Most commonly, these
operations are (orthogonal) range search and nearest neighbour search. For my implementation, I need only the first, and as such nearest neighbour search is omitted in name of the YAGNI principle.

I have implemented this class with performance and GPU-compatibility in mind, and therefore the memory is efficiently laid out wherever possible (although some improvements can still be made later).

The implementation supports a more classical C++ interface where vectors of items are returned, as well as a more functional approach which employs higher-order functions to effect the required computation. This functional approach is the backbone to the classical approach as well.

I believe that a k-d tree implementation is a good thing to have in Acts, since they are very versatile and I can see them being used in several other parts of the code.

Again, I have added tests for the k-d tree as well, to ensure that the implementation works as intended.

This pull request cannot be merged until #985 and #986 are merged.

@stephenswat stephenswat changed the title feat: Implement a _k_-d tree data structure feat: Implement a k-d tree data structure Sep 10, 2021
@stephenswat stephenswat added Component - Core Affects the Core module Feature Development to integrate a new feature labels Sep 10, 2021
@stephenswat stephenswat added this to the next milestone Sep 10, 2021
@stephenswat stephenswat added the 🚧 WIP Work-in-progress label Sep 10, 2021
@stephenswat stephenswat removed the 🚧 WIP Work-in-progress label Sep 13, 2021
@stephenswat stephenswat marked this pull request as ready for review September 13, 2021 15:03
@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #987 (7c95c47) into main (d9c04a6) will increase coverage by 0.06%.
The diff coverage is 56.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #987      +/-   ##
==========================================
+ Coverage   48.67%   48.73%   +0.06%     
==========================================
  Files         336      337       +1     
  Lines       17158    17281     +123     
  Branches     8098     8162      +64     
==========================================
+ Hits         8351     8422      +71     
- Misses       3100     3118      +18     
- Partials     5707     5741      +34     
Impacted Files Coverage Δ
Core/include/Acts/Utilities/KDTree.hpp 56.30% <56.30%> (ø)
Core/include/Acts/Utilities/Range1D.hpp 97.36% <0.00%> (+0.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9c04a6...7c95c47. Read the comment docs.

@stephenswat
Copy link
Member Author

Hmm, that test coverage is quite low. I'll work on improving that.

@stephenswat
Copy link
Member Author

I've updated the commit with the following changes:

  1. The str methods have been renamed to toString methods, just like in feat: Implement a k-dimensional range class #986.
  2. The license header has been fixed.
  3. An undocumented parameter which was causing the docs generation to crash has been documented.
  4. Additional tests have been added to cover some edge cases.
  5. The k-d tree has been modified to work with the new half-open intervals (which was somewhat awkward).

This pull request is part three of four in my attempt to implement an
orthogonal range search seed finder.

k-d trees are datastructures which allow certain operations to be
performed efficiently on point cloud data. Most commonly, these
operations are (orthogonal) range search and nearest neighbour search.
For my implementation, I need only the first, and as such nearest
neighbour search is omitted in name of the YAGNI principle.

I have implemented this class with performance and GPU-compatibility in
mind, and therefore the memory is efficiently laid out wherever
possible (although some improvements can still be made later).

The implementation supports a more classical C++ interface where vectors
of items are returned, as well as a more functional approach which
employs higher-order functions to effect the required computation. This
functional approach is the backbone to the classical approach as well.

I believe that a k-d tree implementation is a good thing to have in
Acts, since they are very versatile and I can see them being used in
several other parts of the code.

Again, I have added tests for the k-d tree as well, to ensure that the
implementation works as intended.
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Very nice to get this together, I saw you fixed the doc compilation already, so that's great.

I had two questions for my understanding, but other than that I am happy to merge this in.

Core/include/Acts/Utilities/KDTree.hpp Show resolved Hide resolved
Core/include/Acts/Utilities/KDTree.hpp Show resolved Hide resolved
@stephenswat
Copy link
Member Author

So I've noticed the Mac OS CI build timed out; is that something that sometimes happens? Is the OS X build just slow?

@asalzburger
Copy link
Contributor

So I've noticed the Mac OS CI build timed out; is that something that sometimes happens? Is the OS X build just slow?

Ha, no idea, let's just re-start the CI and see.

@asalzburger asalzburger merged commit c0db8a7 into acts-project:main Sep 14, 2021
@paulgessinger paulgessinger modified the milestones: next, v13.0.0 Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Feature Development to integrate a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants