Support for raw vectors #2952

romainfrancois · 2017-07-08T16:05:15Z

This adds support for raw vectors across the verbs. I had to modify some tests accordingly.

My use case is for the collage pkg: https://github.com/ThinkRstat/collage where we have raw vectors for red, green and blue channels of colors.

This is related to (but independent of) this purrr pr tidyverse/purrr#345

krlmlr

Thanks, this looks good. Could you please split it (=remove hybrid-related changes), and add a few more tests?

krlmlr · 2017-07-12T22:04:55Z

inst/include/dplyr_RcppExports.h

@@ -73,7 +73,7 @@ namespace dplyr {
        RObject rcpp_result_gen;
        {
            RNGScope RCPP_rngScope_gen;
-            rcpp_result_gen = p_build_index_cpp(Shield<SEXP>(Rcpp::wrap(data)));
+            rcpp_result_gen = p_build_index_cpp(Rcpp::wrap(data));


Do you need to update Rcpp?

krlmlr · 2017-07-12T22:07:39Z

inst/include/dplyr/Collecter.h

+  }
+
+  bool can_promote(SEXP x) const {
+    return


Is this promotion actually tested anywhere?

✔️ in 8bd7a1f

krlmlr · 2017-07-12T22:18:48Z

inst/include/dplyr/MatrixColumnSubsetVectorVisitor.h

+// because RAWSXP does not have the NA concept
+template <>
+template <typename Container>
+inline SEXP MatrixColumnSubsetVisitor<RAWSXP>::subset_int(const Container& index) const {


Do we have a test for this?

I've just added a test for it here: ec3bd34

i.e.

test_that( "slice handles raw matrices", { df <- data.frame( a = 1:4 ) df$b <- matrix(as.raw(1:8), ncol = 2) expect_identical( slice( df, 1:2 )$b, matrix(as.raw(c(1,2,5,6)), ncol = 2) ) })

but matrices are not widely supported, as e.g.

> filter(df, a < 4) Error: Column `b` must be a 1d atomic vector or a list

krlmlr · 2017-07-12T22:20:22Z

inst/include/dplyr/MatrixColumnSubsetVectorVisitor.h

+  Matrix<RAWSXP> res(n, nc);
+  for (int h = 0; h < nc; h++) {
+    Column column = res.column(h);
+    Column source_column = const_cast<Matrix<RAWSXP>&>(data).column(h);


Could we use a ConstColumn here instead of the const_cast? Also in the default implementation?

sure. this is mostly a cry for auto though.

krlmlr · 2017-07-13T05:09:23Z

tests/testthat/helper-astyle.R

@@ -19,7 +19,7 @@ astyle <- function(extra_args = character()) {
    "--align-reference=type"
  )

-  src_path <- normalizePath(map_chr(c("../../src", "../../inst/include"), testthat::test_path))
+  src_path <- normalizePath(purrr::map_chr(c("../../src", "../../inst/include"), testthat::test_path))


Actually, we have defined a "poor man's" map_chr() in this package.

fixed now, sorry about that.

krlmlr · 2017-07-13T05:10:17Z

src/join.cpp

+  {
+    switch (TYPEOF(right.get_data())) {
+      case RAWSXP: {
+        return new JoinVisitorImpl<RAWSXP, RAWSXP, ACCEPT_NA_MATCH> (left, right, warn_);


Do we have a test?

✔️ ec4c9a7

krlmlr · 2017-07-13T05:13:34Z

inst/include/dplyr/Result/Rank.h

@@ -230,6 +230,88 @@ class Rank_Impl : public Result, public Increment {
  Map map;
 };

+template <typename Increment, bool ascending>
+class Rank_Impl<RAWSXP, Increment, ascending> : public Result, public Increment {


Why do we need to specialize this class? Actually, I'd rather review all hybrid-related changes in a separate PR, and keep forwarding raw columns to R evaluation for now.

I dropped it now. The specialization was because there is no notion of NA for raw vectors.
very low hanging 🍉 🍌 🍎 almost no chance we want to rank based one raw vector, so R evaluation is fine.

romainfrancois · 2017-07-15T08:50:16Z

Thanks for the review. I'll revise it when j get a chance.

Class specializations for raw usually are because raw don't have the concept of NA.

I'm not sure this can be split into two PRs.

krlmlr · 2017-07-15T09:05:19Z

Thanks, please take your time.

I'd argue that we don't need to include the hybrid handlers in this PR, because they'll just offload to standard evaluation if they're not implemented.

…parameter`

test for `MatrixColumnSubsetVisitor<RAWSXP>::subset_int`

romainfrancois

@krlmlr I think it looks good now

romainfrancois · 2017-12-13T10:38:13Z

inst/include/dplyr/Collecter.h

+  }
+
+  bool can_promote(SEXP x) const {
+    return


✔️ in 8bd7a1f

romainfrancois · 2017-12-13T11:04:59Z

inst/include/dplyr/MatrixColumnSubsetVectorVisitor.h

+// because RAWSXP does not have the NA concept
+template <>
+template <typename Container>
+inline SEXP MatrixColumnSubsetVisitor<RAWSXP>::subset_int(const Container& index) const {


I've just added a test for it here: ec3bd34

i.e.

test_that( "slice handles raw matrices", { df <- data.frame( a = 1:4 ) df$b <- matrix(as.raw(1:8), ncol = 2) expect_identical( slice( df, 1:2 )$b, matrix(as.raw(c(1,2,5,6)), ncol = 2) ) })

but matrices are not widely supported, as e.g.

> filter(df, a < 4) Error: Column `b` must be a 1d atomic vector or a list

romainfrancois · 2017-12-13T11:13:50Z

inst/include/dplyr/MatrixColumnSubsetVectorVisitor.h

+  Matrix<RAWSXP> res(n, nc);
+  for (int h = 0; h < nc; h++) {
+    Column column = res.column(h);
+    Column source_column = const_cast<Matrix<RAWSXP>&>(data).column(h);


sure. this is mostly a cry for auto though.

romainfrancois · 2017-12-13T11:45:06Z

inst/include/dplyr/Result/Rank.h

@@ -230,6 +230,88 @@ class Rank_Impl : public Result, public Increment {
  Map map;
 };

+template <typename Increment, bool ascending>
+class Rank_Impl<RAWSXP, Increment, ascending> : public Result, public Increment {


I dropped it now. The specialization was because there is no notion of NA for raw vectors.
very low hanging 🍉 🍌 🍎 almost no chance we want to rank based one raw vector, so R evaluation is fine.

romainfrancois · 2017-12-13T11:48:35Z

inst/include/dplyr_RcppExports.h

@@ -73,7 +73,7 @@ namespace dplyr {
        RObject rcpp_result_gen;
        {
            RNGScope RCPP_rngScope_gen;
-            rcpp_result_gen = p_build_index_cpp(Shield<SEXP>(Rcpp::wrap(data)));
+            rcpp_result_gen = p_build_index_cpp(Rcpp::wrap(data));


romainfrancois · 2017-12-13T12:47:27Z

src/join.cpp

+  {
+    switch (TYPEOF(right.get_data())) {
+      case RAWSXP: {
+        return new JoinVisitorImpl<RAWSXP, RAWSXP, ACCEPT_NA_MATCH> (left, right, warn_);


✔️ ec4c9a7

romainfrancois · 2017-12-13T12:49:28Z

tests/testthat/helper-astyle.R

@@ -19,7 +19,7 @@ astyle <- function(extra_args = character()) {
    "--align-reference=type"
  )

-  src_path <- normalizePath(map_chr(c("../../src", "../../inst/include"), testthat::test_path))
+  src_path <- normalizePath(purrr::map_chr(c("../../src", "../../inst/include"), testthat::test_path))


fixed now, sorry about that.

krlmlr

Thanks, I like it. We might be able to make this code slightly more compact in a few places. Could you please rerun astyle, too?

krlmlr · 2017-12-13T22:54:32Z

inst/include/dplyr/Result/Lag.h

+inline Lag<RAWSXP>::Lag(SEXP data_, int n_, const RObject& def_, bool is_summary_) :
+  data(data_),
+  n(n_),
+  def((Rbyte)0),


Can we use the new default_value() here (and also for Lead), instead of overriding the constructor?

krlmlr · 2017-12-13T23:00:30Z

inst/include/dplyr/VectorVisitorImpl.h

+  }
+
+  bool is_na(int) const {
+    return false ;


I wonder if we could define our own is_na() akin to default_value() and use that in the default implementation of VectorVisitorImpl? Or perhaps there's a way to just override VectorVisitorImpl<RAWSXP>::is_na()?

Seems we don't need to because RawVector::is_na already does the right thing.
https://github.com/RcppCore/Rcpp/blob/6f81b4684481dbd9bb554dd95e66725fc3b63a8c/inst/include/Rcpp/traits/is_na.h#L31

However, NumericVector::is_na() does not. https://twitter.com/romain_francois/status/938792189932556288

I'll open an issue to discuss this, it might be one of those cases where we depend on an upstream bug.

I removed the VectorVisitorImpl specialization, and just specialized comparisons for RAWSXP, mostly to get rid of the two if branches in is_less.

I'm seeing:

is.na(NaN) #> [1] TRUE

So the Rcpp version seems consistent with what R is doing. Why doesn't this work for us?

can you echo this on #3250 ?

I guess I have a problem with R then. I thought NA and NaN were two very different concepts, and assumed that is.na would 🍒 pick only NA

…`Lag` constructors instead of override them for `RAWSXP`.

… lower level `comparisons` instead.

romainfrancois · 2017-12-14T07:46:33Z

Done.
Running astyle did not change anything this time, perhaps I'm not using the same version. I have:

$ astyle --version
Artistic Style Version 3.0

Perhaps devtools or one of its implementation 📦 could setup some git hook to run astyle, maybe as part of use_Rcpp ? @hadley ?

krlmlr · 2017-12-14T08:58:47Z

Thanks, I need to revisit the astyle parameters.

I'm not sure astyle is the best tool, it's just what I knew best at that time. Other projects use the clang formatter. I think a Git hook would have its own problems, but we might want to simplify calling astyle or the clang formatter, maybe in the context of the styler package?

krlmlr · 2017-12-21T10:51:58Z

Thanks!

lock · 2018-11-04T09:18:40Z

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

Romain Francois added 3 commits July 8, 2017 11:51

support for raw vectors in mutate and summarise. #1803

de9f177

raw support for arrange, joins (minimal raw x raw), group_by. #1803

ee8bd65

wrong place in NEWS.md

d3184ee

krlmlr reviewed Jul 13, 2017

View reviewed changes

lionel- mentioned this pull request Sep 3, 2017

Not-used column causes dplyr::mutate() to throw exception. #3069

Closed

Romain Francois added 12 commits December 12, 2017 19:18

Merge branch 'master' into raw

f505c08

astyle

a5daba1

up rcpp generated code

d872f36

drop Rank specialization for RAWSXP

cfbf6cd

not naming parameter that is not used, identified by `-Werror=unused-…

364d087

…parameter`

typo s/defaut/default/

847da89

handle raw in grouped and rowwise mutate #1803

b710fa5

test Collecter_Impl::can_promote

8bd7a1f

test that slice handles raw matrices columns.

ec3bd34

test for `MatrixColumnSubsetVisitor<RAWSXP>::subset_int`

using ConstColumn instead of a const_cast

cbfff1f

tests for joins on raw column

ec4c9a7

use poor man's map_chr instead of purrr's

638139e

romainfrancois commented Dec 13, 2017

View reviewed changes

krlmlr reviewed Dec 13, 2017

View reviewed changes

Romain Francois added 2 commits December 14, 2017 08:21

move default_value to its own header file and use it in Lead and …

f0cd1a9

…`Lag` constructors instead of override them for `RAWSXP`.

rm the VectorVisitorImpl<RAWSXP> specialization, and specialize the…

2c2039d

… lower level `comparisons` instead.

pleasing -Werror=unused-parameter once again 🙊

88a66b7

krlmlr requested a review from hadley December 14, 2017 09:17

krlmlr merged commit c04deb3 into master Dec 21, 2017

romainfrancois deleted the raw branch May 8, 2018 07:30

lock bot locked and limited conversation to collaborators Nov 4, 2018

Support for raw vectors #2952

Support for raw vectors #2952

Conversation

romainfrancois commented Jul 8, 2017

krlmlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romainfrancois commented Jul 15, 2017

krlmlr commented Jul 15, 2017

romainfrancois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krlmlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romainfrancois commented Dec 14, 2017

krlmlr commented Dec 14, 2017

krlmlr commented Dec 21, 2017

lock bot commented Nov 4, 2018