Several fixes for IndexType which are not of integral types #154

dav1d-wright · 2021-08-12T14:10:07Z

Hi,

I have attempted to use nanoflann for data structures where the access to the elements is not provided using indexes of integral types, or more efficiently provided by other accessors. While doing so I have found certain issues, most of which arose due to mixing up the type of the values stored in KDTreeBaseClass::vind and the type with which vind is accessed.

In many places this worked fine, as the values stored in vind were typically of integral types well, since they are used to quantify indices in an array or a similar data structure. However, with these fixes nanoflann can be used with data structures that provide access to its elements using other types of accessors (such as pointers for example).

Here is a quick summary of the changes:

Metrics adaptors (L1_Adaptor, L2_Adaptor, ...) did not have a template parameter for IndexType. They expected b_idx to be of size_t, and by doing so casted implicitly to size_t, if possible.
In several places, IndexType was used to store the location in vind in which the IndexType is stored, whereas the type for the argument of std::vector<IndexType>::operator[] has nothing to do with IndexType itself. E.g. KDTreeBaseClass::Node::node_type, KDTreeBaseClass::divideTree.
Since the argument of std::vector<IndexType>::operator[] is not a template parameter, uint64_t was used
As the IndexType is not necessarily an index, I renamed it to AccessorType.

I built and ran all tests as well as the examples (all build targets), each of which passed.

I hope you find these changes useful and will merge them into your repository. If there is anything that I should change, don't hesitate to let me know.

…es that are stored in KDTreeBaseClass::vind with the type of the indices with which the values therein are accessed. In many places this worked fine, as the values stored in vind are of integral types well, since they are used to quantified indices in an array or a similar data structure. However, With these fixes nanoflann can be used with data structures that provide access to its elements using other types of accessors (such as pointers for example). - Metrics adaptors (L1_Adaptor, L2_Adaptor, ...) did not have a template parameter for IndexType. They expected b_idx to be of size_t, and by doing so always casted implicitly to size_t, if possible. - In several places, IndexType was used to store the location in vind in which the IndexType is stored, whereas the type for the argument of std::vector<IndexType>::operator[] has nothing to do with IndexType itself. E.g. KDTreeBaseClass::Node::node_type, KDTreeBaseClass::divideTree. - Since the argument of std::vector<IndexType>::operator[] is not a template parameter, uint64_t was used

jlblancoc

Cool! Apart of my comments, please, add the changes to the Changelog file (even with a link to this PR URL?)

jlblancoc · 2021-08-12T15:49:25Z

include/nanoflann.hpp

@@ -486,17 +492,18 @@ struct SO2_Adaptor {
 * \tparam _DistanceType Type of distance variables (must be signed) (e.g.
 * float, double)


Please, add doxygen docs for the new AccessorType here too?

jlblancoc · 2021-08-12T15:51:05Z

include/nanoflann.hpp

  }

-  void computeMinMax(const Derived &obj, IndexType *ind, IndexType count,
+  void computeMinMax(const Derived &obj, uint64_t ind, uint64_t count,


Shouldn't this uint64_t be AccessorType too? Or if it's not, another new symbolic name?
(I'm doing a quick review, sorry if the answer is obvious)

To my understanding, what was done here is that a pointer to IndexType was used in order to be able to use the element to which ind points to, which is located in vInd (or now vAcc) as the element 0 of an array. However as pointer arithmetic is quite unsafe, I thought it would be somewhat safer and more understandable to explicitly access vAcc with ind as an offset, as we do have access to the array here. By using the std::vector::operator[] explicitly, range checks are at least performed in debug mode.

jlblancoc · 2021-08-12T15:52:07Z

include/nanoflann.hpp

        }
      }
    } else {
-      IndexType idx;
+      uint64_t idx;


Same comment here, and basically, everywhere where uint64_t shows up... it lights a red light for me seen a hard-wired type instead of a typename parameter, a "using A=B;", etc. ;-)

jlblancoc · 2021-08-12T15:53:16Z

include/nanoflann.hpp

      return m_data_matrix.get().rows();
    else
      return m_data_matrix.get().cols();
  }

  // Returns the dim'th component of the idx'th point in the class:
  inline num_t kdtree_get_pt(const IndexType idx, size_t dim) const {
-    if(row_major)
+    if (row_major)


We should add a .clangformat at some point... but that's for another PR!

- Created type alias for Dimension, Size, Offset - Use 32bit types for these aliases, as well as default template arguments for AccessorType - Added doxygen comments for template arguments for metrics adaptors

dav1d-wright · 2021-08-13T09:24:59Z

@jlblancoc Thank you for reviewing my changes! I have addressed your comments, I hope this is roughly what you had in mind. Once these changes are accepted I am more than happy to create a PR with a .clang-format file. I also have a .pre-commit-config.yaml that you could use to to install a pre-commit hook which automatically executes clang-format before each commit.

dav1d-wright · 2021-08-13T09:29:45Z

Also, I am not sure if this is of interest to you as it is a modification of the API, but for our code I prefer to have a unified signature for both, knnSearch and radiusSearch, and therefore changed KNNResultSet to adhere to the same API as RadiusResultSet.

dav1d-wright · 2021-10-05T12:49:50Z

Hi @jlblancoc , did you have a chance to look at my changes with respect to your review comments?

jlblancoc · 2021-10-09T22:23:01Z

Awesome, great contribution, thanks @dav1d-wright !

I'm just curious: in what context would you find useful using e.g. double as an index?? :-) Perhaps I'm so biased by common applications to robotics point clouds.

dav1d-wright · 2021-10-12T11:56:01Z

Thank you very much for merging my pull request @jlblancoc! Very glad you like my proposed changes.

Actually the main motivation for providing AccessorType functionality that is of non-integral type was to enable indexing storage that does not lie in contiguous memory, in which case we use pointers to access the underlying data. We haven't used this with floating point numbers at this point, but I am convinced someone could find a use case for this ;)

dschwen · 2022-01-08T21:20:52Z

examples/pointcloud_kdd_radius.cpp

-		std::vector<size_t>   ret_index(num_results);
+		std::vector<uint32_t>   ret_index(num_results);


I'm puzzled by this change (or rather the change that makes this change necessary). For us it breaks backwards compatibility libMesh/libmesh#3122

Aditionally to that, for msvc there are warnings due to conversion loss with size_t when using the default AccessorType = uint32_t value.

1>C:\dev\GSI\Vstars\packages\nanoflann.1.4.2.14\lib\native\include\nanoflann.hpp(1497,1): warning C4267: '=': conversion from 'size_t' to '_Ty', possible loss of data 1> with 1> [ 1> _Ty=std::_Vbase 1> ]

For KDTreeSingleIndexAdaptor settings the typename AccessorType = uint32_t to size_t leads to some different conversion errors which seem to imply that internally a uint32_t is being used somewhere.

1>C:\dev\GSI\Vstars\packages\nanoflann.1.4.2.14\lib\native\include\nanoflann.hpp(1561,1): warning C4267: 'argument': conversion from 'size_t' to 'const AccessorType', possible loss of data 1> with 1> [ 1> AccessorType=uint32_t 1> ]

David Wright added 2 commits August 12, 2021 15:33

Fixed compile error for examples/pointcloud_kdd_radius.cpp

3e2f07e

jlblancoc requested changes Aug 12, 2021

View reviewed changes

Addressed review comments:

1bac65c

- Created type alias for Dimension, Size, Offset - Use 32bit types for these aliases, as well as default template arguments for AccessorType - Added doxygen comments for template arguments for metrics adaptors

Also use Dimension for divfeat

bef2b48

jlblancoc merged commit 431bd40 into jlblancoc:master Oct 9, 2021

dschwen reviewed Jan 8, 2022

View reviewed changes

lindsayad mentioned this pull request Jan 9, 2022

Nanoflann update to 1.4.1 libMesh/libmesh#3122

Merged

franciszhi mentioned this pull request Mar 24, 2022

Double calls of buildIndex() #168

Closed

jdumas mentioned this pull request Jan 22, 2025

Compatibility with recent nanoflann versions. nmwsharp/geometry-central#207

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several fixes for IndexType which are not of integral types #154

Several fixes for IndexType which are not of integral types #154

dav1d-wright commented Aug 12, 2021 •

edited

Loading

jlblancoc left a comment

jlblancoc Aug 12, 2021

jlblancoc Aug 12, 2021

dav1d-wright Aug 13, 2021

jlblancoc Aug 12, 2021

jlblancoc Aug 12, 2021

dav1d-wright commented Aug 13, 2021

dav1d-wright commented Aug 13, 2021

dav1d-wright commented Oct 5, 2021

jlblancoc commented Oct 9, 2021

dav1d-wright commented Oct 12, 2021 •

edited

Loading

dschwen Jan 8, 2022 •

edited

Loading

cstamatopoulos Jan 24, 2022 •

edited

Loading

		@@ -486,17 +492,18 @@ struct SO2_Adaptor {
		* \tparam _DistanceType Type of distance variables (must be signed) (e.g.
		* float, double)

		std::vector<size_t> ret_index(num_results);
		std::vector<uint32_t> ret_index(num_results);

Several fixes for IndexType which are not of integral types #154

Several fixes for IndexType which are not of integral types #154

Conversation

dav1d-wright commented Aug 12, 2021 • edited Loading

jlblancoc left a comment

Choose a reason for hiding this comment

jlblancoc Aug 12, 2021

Choose a reason for hiding this comment

jlblancoc Aug 12, 2021

Choose a reason for hiding this comment

dav1d-wright Aug 13, 2021

Choose a reason for hiding this comment

jlblancoc Aug 12, 2021

Choose a reason for hiding this comment

jlblancoc Aug 12, 2021

Choose a reason for hiding this comment

dav1d-wright commented Aug 13, 2021

dav1d-wright commented Aug 13, 2021

dav1d-wright commented Oct 5, 2021

jlblancoc commented Oct 9, 2021

dav1d-wright commented Oct 12, 2021 • edited Loading

dschwen Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

cstamatopoulos Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

dav1d-wright commented Aug 12, 2021 •

edited

Loading

dav1d-wright commented Oct 12, 2021 •

edited

Loading

dschwen Jan 8, 2022 •

edited

Loading

cstamatopoulos Jan 24, 2022 •

edited

Loading