NotificationsYou must be signed in to change notification settings
Fork219
Star781

Add assignment operator to DimNameProxy#339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

eddelbuettel merged 6 commits intoRcppCore:masterfromfplazaonate:DimNameProxy-patch1

Aug 14, 2015

Merged

Add assignment operator to DimNameProxy#339

eddelbuettel merged 6 commits intoRcppCore:masterfromfplazaonate:DimNameProxy-patch1

Aug 14, 2015

Conversation

Copy link

Contributor

fplazaonate commentedAug 14, 2015

#include<Rcpp.h>usingnamespaceRcpp;// [[Rcpp::export]]NumericMatrixtest_mat(const NumericMatrix& mat){constint num_rows = mat.rows();constint num_cols = mat.cols();  NumericMatrixres_mat(num_rows, num_cols);rownames(res_mat)=rownames(mat);colnames(res_mat)=colnames(mat);return res_mat;}/*** Rmat=matrix(1:150,nrow=10,ncol=15,byrow=T)rownames(mat)=sapply(1:10,as.character)colnames(mat)=sapply(1:15,as.character)mat_res=test_mat(mat)rownames(mat_res)colnames(mat_res)*/

Before:

> mat=matrix(1:150,nrow=10,ncol=15,byrow=T)> rownames(mat)=sapply(1:10,as.character)> colnames(mat)=sapply(1:15,as.character)> mat_res=test_mat(mat)> rownames(mat_res)NULL> colnames(mat_res)NULL

After:

> mat=matrix(1:150,nrow=10,ncol=15,byrow=T)> rownames(mat)=sapply(1:10,as.character)> colnames(mat)=sapply(1:15,as.character)> mat_res=test_mat(mat)> rownames(mat_res) [1] "1"  "2"  "3"  "4"  "5"  "6"  "7"  "8"  "9"  "10"> colnames(mat_res) [1] "1"  "2"  "3"  "4"  "5"  "6"  "7"  "8"  "9"  "10" "11" "12" "13" "14" "15"

After:

Add assignment operator to DimNameProxy

c5cf125

Copy link

Member

eddelbuettel commentedAug 14, 2015

This looks pretty straightforward -- in favour. Any seconds?

kevinushey reviewed

Aug 14, 2015

View reviewed changes

inst/include/Rcpp/vector/DimNameProxy.h Outdated

Copy link

Contributor

kevinusheyAug 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It looks like you already retrieved this above withSEXP dim = Rf_getAttrib(data_, R_DimSymbol) -- is this line superfluous?

EDIT: I see now the interface is mostly copied from the otheroperator=; I guess that's a similar oversight in the previous implementation.

Copy link

Contributor

romainfrancoisAug 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

And while you're there, now thatstop does theprintf thing, the wholestd::stringstream can be replaced.

Copy link

Contributor

kevinushey commentedAug 14, 2015

I agree this is a good change; it would probably be best to refactor the code into its own function (which bothoperator= methods can call) rather than duplicating code, however.

Florian Plaza Oñate added3 commits

August 14, 2015 16:20

Remove duplicated code

268a5c5

Remove std::stringstream usage

271e03d

Factorize code

3b54aa1

Copy link

ContributorAuthor

fplazaonate commentedAug 14, 2015

I made the change you asked.
Tell me if it suits you.

Copy link

Member

eddelbuettel commentedAug 14, 2015

Looks good to me.

Copy link

ContributorAuthor

fplazaonate commentedAug 14, 2015

#include<Rcpp.h>usingnamespaceRcpp;// [[Rcpp::export]]voidtest_mat(){constint num_rows =2;constint num_cols =2;  NumericMatrixm1(num_rows, num_cols);  NumericMatrixm2(num_rows, num_cols);rownames(m1)=CharacterVector::create("a","b");rownames(m1)=rownames(m2);}/*** Rtest_mat()*/

Result

`test_mat()Error in eval(expr, envir, enclos) :   dimension extent is '2' while length of names is '0'

I think this code should not produce an error as it works in R. What do you think?

Copy link

Contributor

kevinushey commentedAug 14, 2015

I agree that the code shouldn't produce an error; in fact, it looks like in current Rcpp we just get the wrong result:

#include<Rcpp.h>usingnamespaceRcpp;// [[Rcpp::export]]NumericMatrixtest_mat(){constint num_rows =2;constint num_cols =2;   NumericMatrixm1(num_rows, num_cols);   NumericMatrixm2(num_rows, num_cols);rownames(m1)=CharacterVector::create("a","b");rownames(m1)=rownames(m2);return m1;}/*** Rtest_mat()*/

gives

>Rcpp::sourceCpp('~/test.cpp')> test_mat()  [,1] [,2]a00b00

It looks to me like the secondrownames assignment should have cleared the rownames ofm1; if you are willing would you be able to tackle that as well?

Thanks for working on this!

fplazaonate reviewed

Aug 14, 2015

View reviewed changes

inst/include/Rcpp/vector/DimNameProxy.h Outdated

Copy link

ContributorAuthor

fplazaonateAug 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This does the trick

if (Rf_length(other) !=0 && INTEGER(dims)[dim_] != Rf_length(other))

but it looks a bit dirty.

Florian Plaza Oñate added2 commits

August 14, 2015 19:45

Allow to assign empty vector or NULL to dimnames

8b993cf

Fix wrong indent

73974db

Copy link

ContributorAuthor

fplazaonate commentedAug 14, 2015

The last commit fixes the following cases:

#include<Rcpp.h>usingnamespaceRcpp;// [[Rcpp::export]]NumericMatrixtest_mat2(){constint num_rows =2;constint num_cols =2;  NumericMatrixm1(num_rows, num_cols);rownames(m1)=CharacterVector::create('a','b');rownames(m1)=CharacterVector::create();return m1;}// [[Rcpp::export]]NumericMatrixtest_mat(){constint num_rows =2;constint num_cols =2;  NumericMatrixm1(num_rows, num_cols);  NumericMatrixm2(num_rows, num_cols);rownames(m1)=CharacterVector::create("a","b");rownames(m1)=rownames(m2);return m1;}/*** Rtest_mat()test_mat2()*/

> test_mat()     [,1] [,2][1,]    0    0[2,]    0    0> test_mat2()     [,1] [,2][1,]    0    0[2,]    0    0

It is ok to do so in R

> m1=matrix(nrow = 2,ncol = 2)> rownames(m1)=c(1,2)> rownames(m1)[1] "1" "2"> m2=matrix(nrow = 2,ncol = 2)> rownames(m1)=rownames(m2)> rownames(m1)NULL> rownames(m1)=c(1,2)> rownames(m1)=c()> rownames(m1)NULL