xtensor-stack / xtensor-r

R bindings for xtensor
BSD 3-Clause "New" or "Revised" License
87 stars 15 forks source link

Optional support advancements #102

Closed DavisVaughan closed 5 years ago

DavisVaughan commented 5 years ago

Closes #99

SylvainCorlay commented 5 years ago

It seems your forward declaration in the exporter header is not quite the same as the original one.

DavisVaughan commented 5 years ago

Yea I realized that was wrong. I've fixed it locally along with a few other changes. I'm using the new Draft PR feature of github to be able to run the appveyor checks without really saying its ready for review

DavisVaughan commented 5 years ago

Is this really off base? I'm comparing to the other PR where the PreserveStorage class was inherited from, but am a bit out of my depth.

DavisVaughan commented 5 years ago

It still doesn't work right. This always returns NULL for out, but does seem to at least be calling a valid SEXP() fn

// [[Rcpp::depends(xtensor)]]
// [[Rcpp::plugins(cpp14)]]

#include <xtensor-r/rarray.hpp>
#include <xtensor-r/roptional.hpp>
#include <xtensor/xio.hpp>
#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP optional_int(SEXP x) {
  xt::rarray_optional<int> x_rarray(x);

  SEXP out = SEXP(x_rarray);

  return out;
}
DavisVaughan commented 5 years ago

Okay upon further review, I think that your original solution is best. We should just let the rcontainer type handle the special behavior of inheriting from StoragePolicy, and then with the rcontainer_optional SEXP() method we just call out to the SEXP() method of rcontainer. All good.

That makes the example above work fine.

However, things like this still fail to compile:

// [[Rcpp::depends(xtensor)]]
// [[Rcpp::plugins(cpp14)]]

#include <xtensor-r/rarray.hpp>
#include <xtensor-r/roptional.hpp>
#include <xtensor/xio.hpp>
#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP optional_int(SEXP x) {
  xt::rarray_optional<int> x_rarray(x);

  xt::rarray_optional<int> res = xt::sum(x_rarray);

  return x_rarray;
}
[dev]> Rcpp::sourceCpp('~/Desktop/test.cpp')
In file included from test.cpp:4:
In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor-r/rarray.hpp:17:
In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xbuffer_adaptor.hpp:20:
In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xstorage.hpp:21:
In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xtensor_simd.hpp:14:
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xutils.hpp:609:20: error: cannot convert 'xtl::xoptional<int, bool>' to 'int' without a conversion operator
            return static_cast<T>(std::forward<U>(u));
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xutils.hpp:624:16: note: in instantiation of function template specialization 'xt::conditional_cast_functor<true, int>::operator()<xtl::xoptional<int, bool> >' requested here
        return conditional_cast_functor<condition, T>()(std::forward<U>(u));
               ^
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xassign.hpp:473:22: note: in instantiation of function template specialization 'xt::conditional_cast<true, int, xtl::xoptional<int, bool> >' requested here
            *m_lhs = conditional_cast<is_narrowing, result_type>(*m_rhs);
                     ^
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xassign.hpp:319:22: note: in instantiation of member function 'xt::stepper_assigner<xt::rarray<int>, xt::xreducer<xt::xreducer_functors<std::__1::plus<xtl::xoptional<int, bool> >, xt::const_value<xtl::xoptional<int, bool> >, std::__1::plus<xtl::xoptional<int, bool> > >, const xt::rarray<int> &, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>, xt::reducer_options<xtl::xoptional<int, bool>, std::__1::tuple<xt::evaluation_strategy::lazy_type> > >, xt::layout_type::column_major>::run' requested here
            assigner.run();
                     ^
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xoptional.hpp:1129:60: note: in instantiation of function template specialization 'xt::xexpression_assigner_base<xt::xtensor_expression_tag>::assign_data<xt::rarray<int>, xt::xreducer<xt::xreducer_functors<std::__1::plus<xtl::xoptional<int, bool> >, xt::const_value<xtl::xoptional<int, bool> >, std::__1::plus<xtl::xoptional<int, bool> > >, const xt::rarray<int> &, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>, xt::reducer_options<xtl::xoptional<int, bool>, std::__1::tuple<xt::evaluation_strategy::lazy_type> > > >' requested here
        xexpression_assigner_base<xtensor_expression_tag>::assign_data(bde1, xt::value(de2), trivial);
                                                           ^
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xassign.hpp:328:20: note: in instantiation of function template specialization 'xt::xexpression_assigner_base<xt::xoptional_expression_tag>::assign_data<xt::rcontainer_optional<xt::rarray<int> >, xt::xreducer<xt::xreducer_functors<std::__1::plus<xtl::xoptional<int, bool> >, xt::const_value<xtl::xoptional<int, bool> >, std::__1::plus<xtl::xoptional<int, bool> > >, const xt::rcontainer_optional<xt::rarray<int> > &, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>, xt::reducer_options<xtl::xoptional<int, bool>, std::__1::tuple<xt::evaluation_strategy::lazy_type> > > >' requested here
        base_type::assign_data(e1, e2, trivial_broadcast);
                   ^
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xassign.hpp:201:40: note: in instantiation of function template specialization 'xt::xexpression_assigner<xt::xoptional_expression_tag>::assign_xexpression<xt::xexpression<xt::rcontainer_optional<xt::rarray<int> > >, xt::xexpression<xt::xreducer<xt::xreducer_functors<std::__1::plus<xtl::xoptional<int, bool> >, xt::const_value<xtl::xoptional<int, bool> >, std::__1::plus<xtl::xoptional<int, bool> > >, const xt::rcontainer_optional<xt::rarray<int> > &, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>, xt::reducer_options<xtl::xoptional<int, bool>, std::__1::tuple<xt::evaluation_strategy::lazy_type> > > > >' requested here
            xexpression_assigner<tag>::assign_xexpression(e1, e2);
                                       ^
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xsemantic.hpp:587:13: note: in instantiation of function template specialization 'xt::assign_xexpression<xt::rcontainer_optional<xt::rarray<int> >, xt::xreducer<xt::xreducer_functors<std::__1::plus<xtl::xoptional<int, bool> >, xt::const_value<xtl::xoptional<int, bool> >, std::__1::plus<xtl::xoptional<int, bool> > >, const xt::rcontainer_optional<xt::rarray<int> > &, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>, xt::reducer_options<xtl::xoptional<int, bool>, std::__1::tuple<xt::evaluation_strategy::lazy_type> > > >' requested here
        xt::assign_xexpression(*this, e);
            ^
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor/xsemantic.hpp:453:37: note: in instantiation of function template specialization 'xt::xcontainer_semantic<xt::rcontainer_optional<xt::rarray<int> > >::assign_xexpression<xt::xreducer<xt::xreducer_functors<std::__1::plus<xtl::xoptional<int, bool> >, xt::const_value<xtl::xoptional<int, bool> >, std::__1::plus<xtl::xoptional<int, bool> > >, const xt::rcontainer_optional<xt::rarray<int> > &, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>, xt::reducer_options<xtl::xoptional<int, bool>, std::__1::tuple<xt::evaluation_strategy::lazy_type> > > >' requested here
        return this->derived_cast().assign_xexpression(e);
                                    ^
/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include/xtensor-r/roptional.hpp:252:24: note: in instantiation of function template specialization 'xt::xsemantic_base<xt::rcontainer_optional<xt::rarray<int> > >::assign<xt::xreducer<xt::xreducer_functors<std::__1::plus<xtl::xoptional<int, bool> >, xt::const_value<xtl::xoptional<int, bool> >, std::__1::plus<xtl::xoptional<int, bool> > >, const xt::rcontainer_optional<xt::rarray<int> > &, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>, xt::reducer_options<xtl::xoptional<int, bool>, std::__1::tuple<xt::evaluation_strategy::lazy_type> > > >' requested here
        semantic_base::assign(e);
                       ^
test.cpp:14:34: note: in instantiation of function template specialization 'xt::rcontainer_optional<xt::rarray<int> >::rcontainer_optional<xt::xreducer<xt::xreducer_functors<std::__1::plus<xtl::xoptional<int, bool> >, xt::const_value<xtl::xoptional<int, bool> >, std::__1::plus<xtl::xoptional<int, bool> > >, const xt::rcontainer_optional<xt::rarray<int> > &, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>, xt::reducer_options<xtl::xoptional<int, bool>, std::__1::tuple<xt::evaluation_strategy::lazy_type> > > >' requested here
  xt::rarray_optional<int> res = xt::sum(x_rarray);
                                 ^
1 error generated.
make: *** [test.o] Error 1
clang++ -std=gnu++14 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG   -I"/Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include" -I"/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensor/include" -I"/Users/davis/Desktop" -I/usr/local/include   -fPIC  -g -O0 -c test.cpp -o test.o
Error in Rcpp::sourceCpp("~/Desktop/test.cpp") : 
  Error 1 occurred building shared library.

It works with auto res = xt::sum(x_rarray);, but I can't convert to rarray or rarray_optional after that, meaning I can't get the SEXP out after the operation :/

DavisVaughan commented 5 years ago

Would love to get this merged in too, I'm itching to add missing value support to rray. Note that there is still a bug that will probably be handled in another PR (but I don't think I know what the solution is) https://github.com/QuantStack/xtensor-r/pull/102#issuecomment-477654127

SylvainCorlay commented 5 years ago

Would love to get this merged in too, I'm itching to add missing value support to rray. Note that there is still a bug that will probably be handled in another PR (but I don't think I know what the solution is)

102 (comment)

Yes! I just enabled the support for non-vendored xtensor-r in Xtensor.R's CI. I have another PR to test against xtensor-r master.

We should be able that your extra tests pass against master very soon.