-
-
Notifications
You must be signed in to change notification settings - Fork 208
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add assignment operator to DimNameProxy #339
Add assignment operator to DimNameProxy #339
Conversation
This looks pretty straightforward -- in favour. Any seconds? |
stop(s.str()); | ||
} | ||
SEXP dimnames = Rf_getAttrib(data_, R_DimNamesSymbol); | ||
SEXP dims = Rf_getAttrib(data_, R_DimSymbol); |
There was a problem hiding this comment.
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 with SEXP dim = Rf_getAttrib(data_, R_DimSymbol)
-- is this line superfluous?
EDIT: I see now the interface is mostly copied from the other operator=
; I guess that's a similar oversight in the previous implementation.
There was a problem hiding this comment.
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 that stop
does the printf
thing, the whole std::stringstream
can be replaced.
I agree this is a good change; it would probably be best to refactor the code into its own function (which both |
I made the change you asked. |
Looks good to me. |
#include <Rcpp.h>
using namespace Rcpp;
// [[Rcpp::export]]
void test_mat()
{
const int num_rows = 2;
const int num_cols = 2;
NumericMatrix m1(num_rows, num_cols);
NumericMatrix m2(num_rows, num_cols);
rownames(m1)=CharacterVector::create("a", "b");
rownames(m1)=rownames(m2);
}
/*** R
test_mat()
*/ Result
I think this code should not produce an error as it works in R. What do you think? |
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>
using namespace Rcpp;
// [[Rcpp::export]]
NumericMatrix test_mat()
{
const int num_rows = 2;
const int num_cols = 2;
NumericMatrix m1(num_rows, num_cols);
NumericMatrix m2(num_rows, num_cols);
rownames(m1)=CharacterVector::create("a", "b");
rownames(m1)=rownames(m2);
return m1;
}
/*** R
test_mat()
*/ gives > Rcpp::sourceCpp('~/test.cpp')
> test_mat()
[,1] [,2]
a 0 0
b 0 0 It looks to me like the second Thanks for working on this! |
|
||
inline DimNameProxy& assign(SEXP other) { | ||
SEXP dims = Rf_getAttrib(data_, R_DimSymbol); | ||
if (INTEGER(dims)[dim_] != Rf_length(other)) { |
There was a problem hiding this comment.
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.
The last commit fixes the following cases: #include <Rcpp.h>
using namespace Rcpp;
// [[Rcpp::export]]
NumericMatrix test_mat2()
{
const int num_rows = 2;
const int num_cols = 2;
NumericMatrix m1(num_rows, num_cols);
rownames(m1)=CharacterVector::create('a','b');
rownames(m1)=CharacterVector::create();
return m1;
}
// [[Rcpp::export]]
NumericMatrix test_mat()
{
const int num_rows = 2;
const int num_cols = 2;
NumericMatrix m1(num_rows, num_cols);
NumericMatrix m2(num_rows, num_cols);
rownames(m1)=CharacterVector::create("a", "b");
rownames(m1)=rownames(m2);
return m1;
}
/*** R
test_mat()
test_mat2()
*/
It is ok to do so in R
|
} else { | ||
SET_VECTOR_ELT(dimnames, dim_, other); | ||
SEXP dims = Rf_getAttrib(data_, R_DimSymbol); | ||
if (INTEGER(dims)[dim_] != Rf_length(other)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check that dims
is non-null here, e.g. if (dims != R_NilValue && ...)
Thanks! I have one more nitpick re: a null check and then I think it's ready for merge. |
I am not sure it can happen because a DimNameProxy object is currently created only from a Matrix. |
Good point -- okay, then I think this is good to go. @eddelbuettel, any other comments? |
Yup, let's fold this in. |
Add assignment operator to DimNameProxy
Before:
After:
After: