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

Storage container within RadiusResultSet #166

Closed
ivan-pi opened this issue Feb 25, 2022 · 5 comments
Closed

Storage container within RadiusResultSet #166

ivan-pi opened this issue Feb 25, 2022 · 5 comments

Comments

@ivan-pi
Copy link

ivan-pi commented Feb 25, 2022

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().

@jlblancoc
Copy link
Owner

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

@ivan-pi
Copy link
Author

ivan-pi commented Mar 4, 2022

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).

@jlblancoc
Copy link
Owner

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

Exactly, I also thought of the same...

@ivan-pi
Copy link
Author

ivan-pi commented May 17, 2022

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.

@jlblancoc
Copy link
Owner

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

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

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

No branches or pull requests

2 participants