R logical support. Closes #62

DavisVaughan commented 5 years ago

This PR adds logical support to xtensor-r.

Main changes:

There is a fake type, rlogical that will represent R's logical type.
When we ask for the type of rlogical on the R side, we get back LGLSXP (through r_sexptype_traits<rlogical>)
When we ask for the type of rlogical on the xtensor side, we get back int (through r_detail::get_underlying_value_type_r<rlogical>
The type checks in rcontainer.hpp when going R->rcontainer have been updated so they can use rlogical as a proxy for the logical type and error appropriately.

Questions:

There are a few places where I define the underlying_type along with the preexisting value_type (in the constructors of rarray and rtensor) here. At this point, are those the same? I think so, but I wasn't sure because value_type came from storage_type::value_type and I didn't know what storage_type was since it came from xbuffer_adaptor<T>. If they are the same, I think this can be simplified to use the value_type::type* here
Should struct rlogical{} be somewhere else? It needs to be used in both xt and Rcpp so I defined it outside both namespaces. I don't know what the best thing to do here it. It's currently here

To use this, users should use the rlogical type rather than bool. This is working for me in rray

I was also able to coerce something that I knew would come back from xtensor as bool into rlogical and then convert it into an R logical array here, which was nice to see.

wolfv commented 5 years ago

Yeah! :tada: congrats on getting green ticks!

Review coming

DavisVaughan commented 5 years ago

Trying to get things up to date here. I'm getting a few issues with the new master branch.

Ignoring the logical support piece completely, I did the following:

Pull the new xtensor-r master branch
Try and recompile rray with it

I was immediately smacked with:

> devtools::load_all()
Loading rray
Re-compiling rray
─  installing *source* package ‘rray’ ...
   ** libs
   clang++ -std=gnu++14 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -I"/Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include" -I"/Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensorrr/include" -I/usr/local/include   -fPIC  -O3 -c RcppExports.cpp -o RcppExports.o
   In file included from RcppExports.cpp:4:
   In file included from ./../inst/include/rray_types.h:5:
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensorrr/include/xtensor-r/rarray.hpp:143:68: error: type alias 'rstorage' cannot be referenced with a class specifier
           friend class rcontainer<rarray<T>, Rcpp::PreserveStorage>::rstorage;
                                                                      ^
   RcppExports.cpp:54:51: note: in instantiation of template class 'xt::rarray<double>' requested here
       rcpp_result_gen = Rcpp::wrap(rray_reshape_cpp(x, shape));
                                                     ^
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensorrr/include/xtensor-r/rcontainer.hpp:83:15: note: declared here
           using rstorage = SP<D>;
                 ^
   In file included from RcppExports.cpp:4:
   In file included from ./../inst/include/rray_types.h:5:
   In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensorrr/include/xtensor-r/rarray.hpp:23:
   In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensorrr/include/xtensor-r/rcontainer.hpp:23:
   In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include/Rcpp.h:27:
   In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include/RcppCommon.h:125:
   In file included from /Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include/Rcpp/storage/storage.h:4:
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include/Rcpp/storage/PreserveStorage.h:22:40: error: 'update' is a private member of 'xt::rarray<double>'
               static_cast<CLASS&>(*this).update(data) ;
                                          ^
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensorrr/include/xtensor-r/rarray.hpp:149:30: note: in instantiation of member function 'Rcpp::PreserveStorage<xt::rarray<double> >::set__' requested here
           base_type::rstorage::set__(Rcpp::r_cast<SXP>(exp));
                                ^
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensorrr/include/xtensor-r/rcpp_extensions.hpp:41:24: note: in instantiation of member function 'xt::rarray<double>::rarray' requested here
                   return xt::rarray<T>(m_sexp);
                          ^
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include/Rcpp/as.h:89:29: note: in instantiation of member function 'Rcpp::traits::Exporter<xt::rarray<double> >::get' requested here
               return exporter.get();
                               ^
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include/Rcpp/as.h:152:26: note: in instantiation of function template specialization 'Rcpp::internal::as<xt::rarray<double> >' requested here
           return internal::as<T>(x, typename traits::r_type_traits<T>::r_category());
                            ^
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/Rcpp/include/Rcpp/InputParameter.h:34:38: note: in instantiation of function template specialization 'Rcpp::as<xt::rarray<double> >' requested here
           inline operator T() { return as<T>(x) ; }
                                        ^
   RcppExports.cpp:54:51: note: in instantiation of member function 'Rcpp::InputParameter<xt::rarray<double> >::operator rarray' requested here
       rcpp_result_gen = Rcpp::wrap(rray_reshape_cpp(x, shape));
                                                     ^
   /Library/Frameworks/R.framework/Versions/3.5/Resources/library/xtensorrr/include/xtensor-r/rarray.hpp:138:14: note: declared private here
           void update(SEXP new_sexp) noexcept; // called when the stored SEXP changes
                ^
   2 errors generated.
   make: *** [RcppExports.o] Error 1
   ERROR: compilation failed for package ‘rray’
─  removing ‘/private/var/folders/41/qx_9ygp112nfysdfgxcssgwc0000gn/T/RtmprGWILH/devtools_install_50411c849ac9/rray’
Error in run(bin, args = real_cmdargs, stdout_line_callback = real_callback(stdout),  : 
  System command error

I commented out:

//friend class rcontainer<rtensor<T, N>, Rcpp::PreserveStorage>::rstorage;

in both rtensor and rarray files and moved

void update(SEXP new_sexp) noexcept; // called when the stored SEXP changes

from private to public and recompiled and things seem to be working?

I still have not re-worked in the logical support yet. I thought you might want to fix this first.

wolfv commented 5 years ago

I locally checked out your branch, rebased, and seems to work fine! A test of the functionality will be all that's left to do and then we can merge, and release a new version of xtensor-r.

DavisVaughan commented 5 years ago

Alright I've added a test and updated to use the more "modern" code for the rtype.

It should be good to go. (travis looking pretty good with some things passing and some timing out for other reasons)

Separately, I actually don't like that RCPP_WARN_ON_COERCE is being set now. It sets it for all of Rcpp (obviously), and this had some weird implications over in rray. For instance, I was trying to coerce an IntegerVector to a std::vector<std::size_t> using Rcpp::as<>() and it was throwing a warning that I was coercing an integer to a double for some reason. I think that for the most part a lack of warning here is okay. I know in rray I will do any type reconciliation before things are passed on to xtensor, so it wont bother me to turn it back off.

wolfv commented 5 years ago

Cool! I am restarting the failed builds right now.

Regarding the warning: Would you be in favor of turning on a warning limited to xtensor-r? I think it can be confusing if people e.g. pass an integer vector to a function accepting rarray<double>& and then getting a (silent) copy. We basically had such a warning when we were doing the conversion ourselves (or rather not doing any conversion).

Let's finish this PR and then figure out what to do about the warnings situation.

DavisVaughan commented 5 years ago

Would you be in favor of turning on a warning limited to xtensor-r?

Yea I think this would be fine. I think the warning is useful as long as it doesn't propagate to the rest of rcpp. Generally I think R users are used to automatic promotion from integer to double, but the silent copies can be bad and that would be worth warning for.

With rray, when doing a binary operation, I find the common type of the two R types and use that for the inner type of the resulting container, so turning this on shouldn't result in any odd warnings for me. https://github.com/DavisVaughan/rray/blob/master/src/arith.cpp#L10

xtensor-stack / xtensor-r

R logical support. Closes #62 #65