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

Implicit pytensor to xtensor? #1377

Closed
colinfang opened this issue Jan 28, 2019 · 5 comments
Closed

Implicit pytensor to xtensor? #1377

colinfang opened this issue Jan 28, 2019 · 5 comments

Comments

@colinfang
Copy link

From https://xtensor.readthedocs.io/en/latest/bindings.html#full-qualified-api we need to define 2 extra generic functions to bridge pytensor & xtensor

However, it seems the conversion is implicit. The following works.

double test_xtensor2(const xt::xtensor<double, 2>& x){
    return xt::sum(x)();
}

double test_xtensor(const xt::pytensor<double, 2>& x){
    return test_xtensor2(x);
}

m.def("test_xtensor", &test_xtensor, "sum");

Is there anything I need to concern? From my test it doesn't seem extra copy.

@JohanMabille
Copy link
Member

xtensor and pytensor both inherit from xexpression and both have constructor and assign operators that accept an xexpression objetct. What happens here is that a temporary of type xtensor is created from the pytensor (data is copied) and then passed to test_xtensor2.

Removing the const in test_xtensor2 should reveal that copy, it will fail with something like "cannot bind non const reference to temporary"

@colinfang
Copy link
Author

You are absolutely correct. Silly me :(

I do hope we can have a type xtensor_t<T, N> that like what #860 suggests

@colinfang
Copy link
Author

Perhaps xtensor_t isn't easy to build. I saw an open xexpression_dimension pr which seems to have lost momentum. I will go with SFINAE route in this case but it feels cumbersome. Is this a C++ language problem, i.e. no template constraints. (unlike rust trait/generic)

@JohanMabille
Copy link
Member

template constraints in C++ are really primitive for now (basically enable_if), you can add some syntactic sugar, like we did with XTL_REQUIRE, but that's still requires improvements. Concepts and constraints are supposed to land in the C++20.

You can also have a look at this blogpost for solutions about API designs.

@colinfang
Copy link
Author

I ended up with

    template<class C>
    struct is_2d: std::false_type
    {
    };

    template<>
    struct is_2d<xt::xtensor<double, 2>>: std::true_type
    {
    };
    template<>
    struct is_2d<xt::pyensor<double, 2>>: std::true_type
    {
    };

    template<class T>
    using check_2d = std::enable_if_t<is_2d<T>::value, bool>;

    // Prepend the following to the function.
    template<class T, typename = check_2d<T>>
    void compute2(const T& t) {
    }

Maybe xtensor can ship a few traits like check_2d out of box?

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

No branches or pull requests

2 participants