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

Refactor/split kdtree #334

Closed
wants to merge 13 commits into from
Closed

Conversation

minnerbe
Copy link
Contributor

As discussed in #333, I made some attempt to refactor the implementation of KDTree. In particular:

  • Values and positions are now in their own classes (KDTreeValues and KDTreePositions, respectively), thereby also separating storage details of the positions from KDTreeImpl.
  • KDTreeData now is only a container providing easy access to data used in serialization.
  • Creation logic for values and positions is outsourced in their respective classes, meaning that there are only #(value backends) + #(position backends) constructors instead of the #(value backends) * #(position backends) constructors of KDTreeData before.

The benchmarks with 1M points look very similar to those before the refactoring with slightly worse timings. Some of the difference, however, might be explained by my laptop's current temperature and power settings. Unfortunately, I don't have access to a more stable test hardware to do a proper comparison right now.

KDTreeBenchmark.createKDTree            avgt    8  322.834 ± 23.371  ms/op
KDTreeBenchmark.kNearestNeighborSearch  avgt    8   99.910 ±  6.377  ms/op
KDTreeBenchmark.nearestNeighborSearch   avgt    8   64.412 ±  6.320  ms/op
KDTreeBenchmark.radiusNeighborSearch    avgt    8   66.110 ± 11.759  ms/op

Although there are still a few rough edges, I am quite satisfied with how this turned out. In particular, the other searches work without adaptation and for the serialization experiments only very minor and obvious changes are necessary.

What do you think @tpietzsch ?

@tpietzsch
Copy link
Member

I like the split of KDTreePositions into a separate class. KDTreeImpl and the searches only needs that, very neat!

I reverted the split of KDTreeValues. It looked weird to me because it doesn't have much to do with KDTree, it's more "1D RAI like thing", and in the end I just put it back into KDTreeData.

@tpietzsch
Copy link
Member

I merged this into #333

@minnerbe
Copy link
Contributor Author

minnerbe commented Apr 2, 2024

I agree, splitting KDTreeValues out of KDTreeData did look a bit awkward. Thanks for considering this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants