remove Rcpp dependency #313

patperry · 2017-09-29T17:15:40Z

Doing my part to remove C++ from the world. Closes #226.

patperry · 2017-09-29T18:30:59Z

sorry about the failing tests. I'll look into those next week

patperry · 2017-09-29T19:12:04Z

These tests fail on the upstream master, too, I don't think they are anything to do with my code:

> devtools::test(".", "matrix")
Loading tibble
Loading required package: testthat
Testing tibble
matrix: ...............

DONE ===========================================================================

krlmlr

Thanks a lot for taking the time to convert the C++ code.

krlmlr · 2017-10-04T14:51:30Z

src/matrixToDataFrame.c

+    //
+    // but there's no need to get fancy since log10(2^64) = 19.266
+    ndigit = 20;
+    buf = R_alloc(1 + ndigit + 1, 1); // V + (number) + NUL


Maybe just char buf[1 + ndigit + 1]?

krlmlr · 2017-10-04T14:52:55Z

src/matrixToDataFrame.c

+    if (TYPEOF(dimnames) == VECSXP && XLENGTH(dimnames) == 2) {
+        colnames = VECTOR_ELT(dimnames, 1);
+        if (TYPEOF(colnames) == STRSXP) {
+            goto Return;


I'd prefer a solution without goto here. Maybe a separate function get_colnames_from_dimnames() with early return?

krlmlr · 2017-10-04T14:53:39Z

src/matrixToDataFrame.c

+
+    // use the cached value of the type rather than re-computing
+    // (this might be an unnecessary optimization)
+    assert(TYPEOF(x) == type);


Extracting the type from x seems cleaner to me, too.

krlmlr · 2017-10-04T15:00:50Z

src/matrixToDataFrame.c

+
+static SEXP get_class(void)
+{
+	SEXP cls;


Please check indent.

krlmlr · 2017-10-04T15:05:59Z

src/matrixToDataFrame.c

+    SEXP dimnames, rownames;
+    int nprot = 0;
+
+#ifndef TIBBLE_DROP_ROWNAMES


I'm fine with keeping existing row names here. Could you please add a test?

Actually, no: let's keep row names out: #288 (comment). Happy to support a rownames argument in as_tibble.matrix(), which then could be extended to as_tibble.data.frame(). (The concept of keys I brought up in the linked reference is orthogonal, we need to store the data in a column anyway, with or without keys.)

I left them in, but feel free to edit. It seems cleaner to me to handle all the row names logic in just one function. Just do

as_tibble.matrix <- function(x, ...) { as_tibble(matrixToDataFrame(x), ...) }

krlmlr · 2017-10-04T15:10:07Z

src/matrixToDataFrame.c

+    R_xlen_t nrow = 0, ncol = 0;
+    int nprot = 0;
+
+    PROTECT(dim = getAttrib(x, R_DimSymbol)); nprot++;


Can we use Rf_getAttrib() instead of getAttrib() here, to make the use of the R API more obvious? Also for the other functions? Please define R_NO_REMAP to turn off availability of functions without Rf_ prefix.

krlmlr · 2017-10-04T15:10:36Z

src/matrixToDataFrame.c

+    if (TYPEOF(dimnames) == VECSXP && XLENGTH(dimnames) == 2) {
+        rownames = VECTOR_ELT(dimnames, 0);
+        if (TYPEOF(rownames) == STRSXP) {
+            goto Return;


goto again.

krlmlr · 2017-10-04T17:06:17Z

Thanks!

@patperry

- In `glimpse()`, compute `type_sum()` from data frame for dbplyr compatibility (#328). - `as_tibble.matrix()` repairs column names. - Compatible with R 3.1 (#323). - Make add_case() and alias for add_row() (#324, @LaDilettante). - `add_column()` to an empty zero-row tibble with a variable of nonzero length now produces a correct error message (#319). - Logical indexes are supported, a warning is raised if the length does not match the number of rows or 1 (#318). - Tibbles now support character subsetting (#312). - `as_tibble()` gains `rownames` argument (#288, #289). - Remove Rcpp dependency (#313, @patperry). - ``` `[.tbl_df`() ``` supports `drop = TRUE` and omits the warning if `j` is passed. The calls `df[i, j, drop = TRUE]` and `df[i, drop = TRUE]` are now compatible with data frames again (#307, #311). - Prevent `add_column()` from dropping classes and attributes by removing the use of `cbind()`. Additionally this ensures that `add_column()` can be used with grouped data frames (#303, @DavisVaughan). - Fixed width for word wrapping of the extra information (#301).

@DavisVaughan

New formatting -------------- The new pillar package is now responsible for formatting tibbles. Pillar will try to display as many columns as possible, if necessary truncating or shortening the output. Colored output highlights important information and guides the eye. The vignette in the tibble package describes how to adapt custom data types for optimal display in a tibble. New features ------------ - Make `add_case()` an alias for `add_row()` (#324, @LaDilettante). - `as_tibble()` gains `rownames` argument (#288, #289). - `as_tibble.matrix()` repairs column names. - Tibbles now support character subsetting (#312). - ``` `[.tbl_df`() ``` supports `drop = TRUE` and omits the warning if `j` is passed. The calls `df[i, j, drop = TRUE]` and `df[i, drop = TRUE]` are now compatible with data frames again (#307, #311). Bug fixes --------- - Improved compatibility with remote data sources for `glimpse()` (#328). - Logical indexes are supported, a warning is raised if the length does not match the number of rows or 1 (#318). - Fixed width for word wrapping of the extra information (#301). - Prevent `add_column()` from dropping classes and attributes by removing the use of `cbind()`. Additionally this ensures that `add_column()` can be used with grouped data frames (#303, @DavisVaughan). - `add_column()` to an empty zero-row tibble with a variable of nonzero length now produces a correct error message (#319). Internal changes ---------------- - Reexporting `has_name()` from rlang, instead of forwarding, to avoid warning when importing both rlang and tibble. - Compatible with R 3.1 (#323). - Remove Rcpp dependency (#313, @patperry).

patperry added 2 commits September 29, 2017 13:12

remove Rcpp dependency

06029f6

fix build

2b8af67

patperry added 4 commits September 29, 2017 15:29

add missing header; whitespace

49bb342

no need for fancy log10 when computing number of digits

63710d7

refactor; add option to keep row names

f6898b4

add missing break statement

891744f

krlmlr reviewed Oct 4, 2017

View reviewed changes

fix @krmlrr's issues

eedc2c3

krlmlr merged commit 7c9ce6d into tidyverse:master Oct 4, 2017

github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2020

remove Rcpp dependency #313

remove Rcpp dependency #313

patperry commented Sep 29, 2017

patperry commented Sep 29, 2017

patperry commented Sep 29, 2017

krlmlr left a comment

krlmlr Oct 4, 2017

krlmlr Oct 4, 2017

krlmlr Oct 4, 2017

krlmlr Oct 4, 2017

krlmlr Oct 4, 2017

krlmlr Oct 4, 2017

patperry Oct 4, 2017

krlmlr Oct 4, 2017

krlmlr Oct 4, 2017

krlmlr commented Oct 4, 2017

remove Rcpp dependency #313

remove Rcpp dependency #313

Conversation

patperry commented Sep 29, 2017

patperry commented Sep 29, 2017

patperry commented Sep 29, 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

krlmlr commented Oct 4, 2017