nanoflann icon indicating copy to clipboard operation
nanoflann copied to clipboard

Storage container within RadiusResultSet

Open ivan-pi opened this issue 2 years ago • 4 comments

Would it be possible to replace the m_indices_dists container within RadiusResultSet of type std::vector<std::pair<IndexType, DistanceType>> with something that interoperates better with C? Currently, any attempt to use nanoflann from a language other than C++ will probably have to make a copy of the results.

The KNNResultSet is much easier to interoperate with since it simply uses two arrays as storage, i.e.

    IndexType*    indices;
    DistanceType* dists;

One can easily pass a contiguous array from C, Python, or Fortran.

I understand the motivation for using std::vector because in the radius search we generally don't know the number of points that will be found in a query, hence the need for a dynamic container. One simple solution would be to simply replace std::pair with a struct:

struct { 
IndexType idx; 
DistanceType dist;
};

Then we can recover a C-interoperable array of structs via m_indices_dists.data().

ivan-pi avatar Feb 25 '22 19:02 ivan-pi

Fair point. Feel free of proposing a PR with this idea and will review it. Ideally, maintaining backwards compatibility as much as possible...

jlblancoc avatar Mar 04 '22 08:03 jlblancoc

A plan I had in mind was to introduce a new template parameter RadiusResultItem of default type std::pair<IndexType, DistanceType> in the RadiusResultSet class. Consumers from C could then use the struct version instead; they need to flatten out the template parameters anyways. I just need to figure out how to make the access to the underlying float and integer values generic (perhaps by naming them first and second just like in the std::pair).

ivan-pi avatar Mar 04 '22 10:03 ivan-pi

(perhaps by naming them first and second just like in the std::pair).

Exactly, I also thought of the same...

jlblancoc avatar Mar 07 '22 09:03 jlblancoc

I've noticed today it's possible to access the radius results from a C-like struct array with a bit of "cheating" as both std::pair and the trivial struct are standard layout types. According to cppreference.com

A pointer to a standard-layout class may be converted (with reinterpret_cast) to a pointer to its first non-static data member and vice versa.

A small demonstration is shown below:

/* radiusresult.cpp */
#include <iostream>
#include <type_traits>
#include <utility>
#include <vector>

typedef std::pair<int,double> RadiusResultItem;

struct RadiusResultItem_C {
    int first;
    double second;
};

int main(int argc, char const *argv[])
{
    // both the std::pair struct and the C-style struct are standard layout 
    std::cout << "is_standard_layout<RadiusResultItem>   " << std::is_standard_layout<RadiusResultItem>::value << '\n';
    std::cout << "is_standard_layout<RadiusResultItem_C> " << std::is_standard_layout<RadiusResultItem_C>::value << '\n';

    std::vector<RadiusResultItem> r(3);

    r[0] = RadiusResultItem(1, 1.0);
    r[1] = RadiusResultItem(2, 4.0);
    r[2] = RadiusResultItem(3, 9.0);

    // Cast the vector to a C-like struct
    RadiusResultItem_C *rc;
    rc = reinterpret_cast<RadiusResultItem_C *>(r.data());

    std::cout << "r.size " << r.size() << '\n';

    for (size_t i = 0; i < r.size(); i++) {
        std::cout << "item[" << i << "]: " << rc[i].first << ' ' << rc[i].second << '\n';       
    }

    return 0;
}
$ g++ -Wall radiusresult.cpp 
$ ./a.out
is_standard_layout<RadiusResultItem>   1
is_standard_layout<RadiusResultItem_C> 1
r.size 3
item[0]: 1 1
item[1]: 2 4
item[2]: 3 9

I'm not sure if it can be guaranteed that std::pair is standard layout for any default integer and floating-point types, and whether the memory layout is fixed or it could be implementation specific.

If not, the best one can do is document it clearly, and provide unit-tests to make sure, the layouts are compatible. I'm speaking here for mixed-language projects in C/Fortran/Python which would like to use nanoflann.

ivan-pi avatar May 17 '22 08:05 ivan-pi

@ivan-pi It was a long time... but this idea is now implemented in fe644599c40ae346d5c816aead6dca1b360d7c60

Thanks for the pointer! Hopefully it will ease the integration with other languages now.

jlblancoc avatar Nov 17 '22 23:11 jlblancoc