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

[SpatialPartitioning] Refactor KdTree into KdTreeDense + KdTreeSparse #129

Merged
merged 4 commits into from
Dec 21, 2023

Conversation

azaleostu
Copy link
Contributor

@azaleostu azaleostu commented Dec 18, 2023

This PR moves the subsampling API of the kdtree into a separate type (KdTreeSparse). Having two separate types for regular kdtrees and subsampled kdtrees will make it easier to implement inverse sample mapping (i.e. mapping point indices to sample indices) which we need to allow constructing KNN graphs from subsampled kdtrees (see #114).

Changes

  • Separate the KdTreeBase class into a KdTreeImplBase class containing common features and two derived KdTreeDenseBase and KdTreeSparseBase classes
  • Add utility functions for converting sample indices to their corresponding point index/point data, see pointFromSample and pointDataFromSample

Ponca/src/SpatialPartitioning/KdTree/kdTree.h Outdated Show resolved Hide resolved
Ponca/src/SpatialPartitioning/KdTree/kdTree.h Outdated Show resolved Hide resolved
Ponca/src/SpatialPartitioning/KdTree/kdTree.h Outdated Show resolved Hide resolved
Ponca/src/SpatialPartitioning/KdTree/kdTree.hpp Outdated Show resolved Hide resolved
@nmellado nmellado linked an issue Dec 18, 2023 that may be closed by this pull request
@azaleostu azaleostu force-pushed the kdtree-refactor branch 9 times, most recently from eed0b9b to 5b08bd4 Compare December 19, 2023 14:35
@azaleostu azaleostu changed the title [SpatialPartitioning] Refactor KdTree into KdTree + KdTreeLod [SpatialPartitioning] Refactor KdTree into KdTreeDense + KdTreeSparse Dec 19, 2023
@azaleostu azaleostu force-pushed the kdtree-refactor branch 3 times, most recently from c3ac7d5 to 518fed8 Compare December 19, 2023 15:01
Copy link
Contributor

@nmellado nmellado left a comment

Choose a reason for hiding this comment

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

From a user perspective, the class KdTreeImpl is frequently used to access types and take objects as inputs in functions/methods. In this context, the name KdTreeImpl sounds here a bit weird, KdTreeBase would make more sense.
Maybe we could update the names in the inheritance hierarchy, in order to help users understand the meaning of each class (especially the ones used downstream).

Ponca/src/SpatialPartitioning/KdTree/kdTree.h Outdated Show resolved Hide resolved
Ponca/src/SpatialPartitioning/KdTree/kdTree.h Show resolved Hide resolved
Ponca/src/SpatialPartitioning/KdTree/kdTree.h Show resolved Hide resolved
doc/src/ponca_module_spatialpartitioning.mdoc Outdated Show resolved Hide resolved
@nmellado
Copy link
Contributor

From a user perspective, the class KdTreeImpl is frequently used to access types and take objects as inputs in functions/methods. In this context, the name KdTreeImpl sounds here a bit weird, KdTreeBase would make more sense. Maybe we could update the names in the inheritance hierarchy, in order to help users understand the meaning of each class (especially the ones used downstream).

@azaleostu did you see this comment ?

@mazestic
Copy link
Contributor

mazestic commented Dec 21, 2023

From a user perspective, the class KdTreeImpl is frequently used to access types and take objects as inputs in functions/methods. In this context, the name KdTreeImpl sounds here a bit weird, KdTreeBase would make more sense. Maybe we could update the names in the inheritance hierarchy, in order to help users understand the meaning of each class (especially the ones used downstream).

@azaleostu did you see this comment ?

yes! so renaming the KdTreeImplBase class to KdTreeBase, and renaming the KdTreeImpl typedef to KdTree?

@nmellado
Copy link
Contributor

From a user perspective, the class KdTreeImpl is frequently used to access types and take objects as inputs in functions/methods. In this context, the name KdTreeImpl sounds here a bit weird, KdTreeBase would make more sense. Maybe we could update the names in the inheritance hierarchy, in order to help users understand the meaning of each class (especially the ones used downstream).

@azaleostu did you see this comment ?

yes! so renaming the KdTreeImplBase class to KdTreeBase, and renaming the KdTreeImpl typedef to KdTree?

Seems to be more user friendly, yes. Are you fine with it ?

@nmellado nmellado merged commit a75f4aa into poncateam:master Dec 21, 2023
12 checks passed
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.

Ambiguities associated to the KdTree Sampling construction scheme
3 participants