DataFrame converting to list on push_back with some c++ standards/compilers

[waltersom]

I want to construct a data frame in Rcpp, with some variable number of columns. To do this, I am using push_back().

However, depending on compiler options, this results in the data frame converting into a list. Something similar was observed back in 2011 in https://stackoverflow.com/questions/8631197/constructing-a-data-frame-in-rcpp.

If I have CXX=g++ --std=gnu++11 in my ~/.R/Makevars (the default; alternatively, if I leave ~/.R/Makevars empty, or clang++ with any standard), I get the following

library(Rcpp)

A <- cppFunction(code='DataFrame make_df() {
NumericVector v(1);
DataFrame df = DataFrame::create(_["v"] = v);
return(df);}', rebuild = TRUE)

B <- cppFunction(code='DataFrame make_df() {
NumericVector v(1);
DataFrame df = DataFrame();
df.push_back(v, "v");
return(df);}', rebuild = TRUE)

A() # Constructed with create()
#>   v
#> 1 0
B() # Constructed with push_back()
#> $v
#> [1] 0

^{Created on 2020-06-25 by the reprex package (v0.3.0)}

I would expect that A() and B() are the same.

If I specify any other standard to g++ (I tried gnu++14, gnu++17, gnu++2a, not specifying a standard), then I get the following, which is what I would expect

library(Rcpp)

A <- cppFunction(code='DataFrame make_df() {
NumericVector v(1);
DataFrame df = DataFrame::create(_["v"] = v);
return(df);}', rebuild = TRUE)

B <- cppFunction(code='DataFrame make_df() {
NumericVector v(1);
DataFrame df = DataFrame();
df.push_back(v, "v");
return(df);}', rebuild = TRUE)

A() # Constructed with create()
#>   v
#> 1 0
B() # Constructed with push_back()
#>   v
#> 1 0

^{Created on 2020-06-25 by the reprex package (v0.3.0)}

I know that I can use as.data.frame() on the result, or Rcpp::as<Rcpp::DataFrame>(), to convert back to a data frame to get what I want, but this compiler-dependent behaviour seems like a bug.

sessionInfo()
#> R version 4.0.0 (2020-04-24)
#> Platform: x86_64-pc-linux-gnu (64-bit)
#> Running under: Debian GNU/Linux bullseye/sid
#> 
#> Matrix products: default
#> BLAS:   /home/walter/R/lib/R/lib/libRblas.so
#> LAPACK: /home/walter/R/lib/R/lib/libRlapack.so
#> 
#> locale:
#>  [1] LC_CTYPE=en_NZ.UTF-8       LC_NUMERIC=C              
#>  [3] LC_TIME=en_NZ.UTF-8        LC_COLLATE=en_NZ.UTF-8    
#>  [5] LC_MONETARY=en_NZ.UTF-8    LC_MESSAGES=en_NZ.UTF-8   
#>  [7] LC_PAPER=en_NZ.UTF-8       LC_NAME=C                 
#>  [9] LC_ADDRESS=C               LC_TELEPHONE=C            
#> [11] LC_MEASUREMENT=en_NZ.UTF-8 LC_IDENTIFICATION=C       
#> 
#> attached base packages:
#> [1] stats     graphics  grDevices utils     datasets  methods   base     
#> 
#> loaded via a namespace (and not attached):
#>  [1] compiler_4.0.0  magrittr_1.5    tools_4.0.0     htmltools_0.4.0
#>  [5] yaml_2.2.1      Rcpp_1.0.4.6    stringi_1.4.6   rmarkdown_2.1  
#>  [9] highr_0.8       knitr_1.28      stringr_1.4.0   xfun_0.14      
#> [13] digest_0.6.25   rlang_0.4.6     evaluate_0.14

^{Created on 2020-06-25 by the reprex package (v0.3.0)}

[eddelbuettel]

The issue with growing a data.frame has been known since basically day one; I was unaware of side effects from newer compilation standards. I can't really see why it matters as all this should be old baselibe C++98/C++03 code. But if you want to take a look, go for it.

Otherwise growing a list and converting at the end is quite natural, and the use of the ::create() method is common too.

[waltersom]

I have been playing around a bit trying to work out what's going on, and making some (very) slow progress.

It seems to me that there is an additional call to DataFrame_Impl on return (to wrapper function in eg RcppExports.cpp) with c++14 or later.

This results in conversion back to a data frame, which doesn't happen for earlier (c++11 or earlier) standards.

Rcpp/inst/include/Rcpp/DataFrame.h

Lines 100 to 107 in 97222bb

	void set__(SEXP x){
	if( ::Rf_inherits( x, "data.frame" )){
	Parent::set__( x ) ;
	} else{
	Shield<SEXP> y(internal::convert_using_rfunction( x, "as.data.frame" )) ;
	Parent::set__( y ) ;
	}
	}

(That is, on push_back, the data frame loses its class always, but with the newer standards, it gets converted back when the function returns, AFAICT).

[eddelbuettel]

Interesting. I can't (immediatly) see how as we don't make explicit alloweance for C++14 or later. Hm.

But I really appreciate that you are digging. The 'have to call as.data.frame' approach always was a little clunky, if you find a way to smooth it I'd be all ears.

[waltersom]

A bit more progress!
Compiling with -fno-elide-constructors yields the same behaviour on all standards (with g++, haven't got onto clang yet...).

Even better, that behaviour is the one I think everyone would prefer, with the extra call to set__, and hence it returns a data frame, not (just) a list.

[eddelbuettel]

Very very interesting. I don't think we can enforce such compiler switches but it may open a door to changing the code...

[kevinushey]

Nice sleuthing -- I think this means we're depending on the implicit copy constructor to "fix" the associated class?

The push_back() implementation eventually ends up here:

Rcpp/inst/include/Rcpp/vector/Vector.h

Lines 675 to 701 in 97222bb

	void push_back_name__impl(const stored_type& object, const std::string& name, traits::true_type ) {
	Shield<SEXP> object_sexp( object ) ;
	R_xlen_t n = size() ;
	Vector target( n + 1 ) ;
	iterator target_it( target.begin() ) ;
	iterator it(begin()) ;
	iterator this_end(end());
	SEXP names = RCPP_GET_NAMES(Storage::get__()) ;
	Shield<SEXP> newnames( ::Rf_allocVector( STRSXP, n+1 ) ) ;
	int i=0;
	if( Rf_isNull(names) ){
	for( ; it < this_end; ++it, ++target_it,i++ ){
	*target_it = *it ; // #nocov
	SET_STRING_ELT( newnames, i , R_BlankString ); // #nocov
	}
	} else {
	for( ; it < this_end; ++it, ++target_it, i++ ){
	*target_it = *it ;
	SET_STRING_ELT( newnames, i, STRING_ELT(names, i ) ) ;
	}
	}
	SET_STRING_ELT( newnames, i, Rf_mkChar( name.c_str() ) );
	target.attr("names") = newnames ;
	*target_it = object_sexp;
	Storage::set__( target.get__() ) ;
	}

and note that it basically works by creating a brand new vector of size n + 1, and then copying elements to that new vector. I guess because this is just a plain new Vector without any explicit class set, we end up creating a plain R list without copying the necessary attributes to make it a real R data.frame. That only gets "fixed" if the implicit copy constructor is called, but that copy can be (and normally is) elided with newer compilers.

I'm less sure what the right fix is, though. The "simplest" fix would be to provide overrides for these methods in the DataFrame class, I suppose?

[waltersom]

Maybe push_back_name__impl could copy over attributes? I guess it's not just data frames that are affected here, but any object with attributes that should be preserved. As it is, names are explicitly copied over, but nothing else.

[waltersom]

I almost have something working, by implementing push_back and push_front in DataFrame.h; this takes care of setting the class, row.names, for the new object (basically what @kevinushey suggested).

I was toying with the idea of Vector copying attributes, as that would be a more general solution, but it's not clear to me how one can tell which attributes should be expanded (eg names), and which straight copied over (class, row.names?).

One question, what is the expected behaviour of

NumericVector v1(1); 
NumericVector v2(2);
DataFrame df = DataFrame::create();
df.push_back(v1, "v1"); //one row
df.push_back(v2, "v2"); //two rows?

?

The R equivalent is

library(magrittr)
data.frame() %>% cbind(v1=c(1))
#> Error in data.frame(..., check.names = FALSE): arguments imply differing number of rows: 0, 1

^{Created on 2020-07-01 by the reprex package (v0.3.0)}

At the moment in Rcpp, it will happily do it, and pad with zeroes, because the push_backs get turned into lists

library(Rcpp)
add_different_lengths <- Rcpp::cppFunction(code='
DataFrame d(){
NumericVector v1(1); 
NumericVector v2(2);
DataFrame df = DataFrame::create();
df.push_back(v1, "v1"); //one row
df.push_back(v2, "v2"); //two rows?
return(df);}')
add_different_lengths()
#> $v1
#> [1] 0
#> 
#> $v2
#> [1] 0 0
as.data.frame(add_different_lengths())
#>   v1 v2
#> 1  0  0
#> 2  0  0

^{Created on 2020-07-01 by the reprex package (v0.3.0)}

I feel that enforcing the component vectors to be the same length is not unreasonable, but perhaps users rely on that not being true?

Edit: this is a bit different if one of the vectors is length one, and the other is non-zero; then the shorter is repeated

[eddelbuettel]

(Your first MCVE failed.)

I think you should not assume a rbind operation. A data.frame is a list of vectors. Growing them by row ... is a day-2 item. I wouldn't worry.

As for attributes, to a fist approximation maybe just pass them along, i.e. copy them?