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

Matrix assigning to double changes #563

Closed
eddelbuettel opened this Issue Oct 22, 2016 · 20 comments

Comments

Projects
None yet
3 participants
@eddelbuettel
Member

eddelbuettel commented Oct 22, 2016

Related to #562, this seems like a bug:

R> cppFunction("NumericMatrix testmat(int n, double fillme) { NumericMatrix M(n, n); M = fillme; return M; }") 
R> testmat(2L, 3)
     [,1] [,2] [,3]
[1,]    0    0    0
[2,]    0    0    0
[3,]    0    0    0
R> 

I was expecting a 2x2 matrix will with the value 3. No mas.

@thirdwing

This comment has been minimized.

Show comment
Hide comment
@thirdwing

thirdwing Oct 22, 2016

Member

I think something like template <typename T> Matrix& operator=( const T& x) is missing.

This is not hard to fix, since we already have this in Vector. But I think there is something more we need to discuss.

In the Vector class, operator= is not used for filling. Maybe we should keep this consistent in Matrix.

#include<Rcpp.h>

using namespace Rcpp;

//[[Rcpp::export]]
NumericVector testvec(int n, double fillme) {
  NumericVector M(n);
  M = fillme;
  return M;
}

/*
> Rcpp::sourceCpp("test.cpp")
> testvec(2, 3)
[1] 3
*/
Member

thirdwing commented Oct 22, 2016

I think something like template <typename T> Matrix& operator=( const T& x) is missing.

This is not hard to fix, since we already have this in Vector. But I think there is something more we need to discuss.

In the Vector class, operator= is not used for filling. Maybe we should keep this consistent in Matrix.

#include<Rcpp.h>

using namespace Rcpp;

//[[Rcpp::export]]
NumericVector testvec(int n, double fillme) {
  NumericVector M(n);
  M = fillme;
  return M;
}

/*
> Rcpp::sourceCpp("test.cpp")
> testvec(2, 3)
[1] 3
*/
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 22, 2016

Member

Great comments, and even better catch. That is dicey. Isn't the dimension change a bug?

Member

eddelbuettel commented Oct 22, 2016

Great comments, and even better catch. That is dicey. Isn't the dimension change a bug?

@thirdwing

This comment has been minimized.

Show comment
Hide comment
@thirdwing

thirdwing Oct 22, 2016

Member

Since we only have Matrix& operator=(const Matrix& other), so I think the compiler will try to convert fillme into a Matrix by calling the ctor Matrix( const int& n).

The we have a 3x3 matrix to assign to M.

Member

thirdwing commented Oct 22, 2016

Since we only have Matrix& operator=(const Matrix& other), so I think the compiler will try to convert fillme into a Matrix by calling the ctor Matrix( const int& n).

The we have a 3x3 matrix to assign to M.

thirdwing pushed a commit to thirdwing/Rcpp that referenced this issue Oct 22, 2016

@thirdwing

This comment has been minimized.

Show comment
Hide comment
@thirdwing

thirdwing Oct 24, 2016

Member

After our discussion in #565 , I think we need to document this behavior.

Member

thirdwing commented Oct 24, 2016

After our discussion in #565 , I think we need to document this behavior.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Member

Rcpp-FAQ entry, maybe?

Member

eddelbuettel commented Oct 24, 2016

Rcpp-FAQ entry, maybe?

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Oct 24, 2016

Contributor

In a section called.... ;-)

Contributor

coatless commented Oct 24, 2016

In a section called.... ;-)

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Member

"Gotchas" ;-)

Just kidding. Maybe among the smorgasbord of examples? Making it 3.13 ?

Member

eddelbuettel commented Oct 24, 2016

"Gotchas" ;-)

Just kidding. Maybe among the smorgasbord of examples? Making it 3.13 ?

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Oct 24, 2016

Contributor

I'm not sure "Examples" portrays the right optics...

"Known Behaviors"? "Known Issues"? "Documented Issues" ?

At this stage, it's semantics for a list of other topics at the top of #506 that need a home as well.

Contributor

coatless commented Oct 24, 2016

I'm not sure "Examples" portrays the right optics...

"Known Behaviors"? "Known Issues"? "Documented Issues" ?

At this stage, it's semantics for a list of other topics at the top of #506 that need a home as well.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Member

All other choise are worse, aren't they?

We cannot make Section 4 a new Section 5 as references break. And a new Section 5 'Warts and Other Unpleasant Things' past Section 4 is also weird.

If you feel you cannot morally contribute in Section 3 just enjoy your evening off or do something else.

If you must, "Section 5: Issues" has to be created with a content set of cardinality one. We could however add an entry there for the eternal issue "Rcpp changed the vector I passed by value" with a very short answer "no you didn't" 😀

Member

eddelbuettel commented Oct 24, 2016

All other choise are worse, aren't they?

We cannot make Section 4 a new Section 5 as references break. And a new Section 5 'Warts and Other Unpleasant Things' past Section 4 is also weird.

If you feel you cannot morally contribute in Section 3 just enjoy your evening off or do something else.

If you must, "Section 5: Issues" has to be created with a content set of cardinality one. We could however add an entry there for the eternal issue "Rcpp changed the vector I passed by value" with a very short answer "no you didn't" 😀

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Oct 24, 2016

Contributor

@eddelbuettel: You know I am going to include that on the PR for the first documented issued.

How would you feel about a subsection within examples? Basically, it's not a moral objection, more so I'm worried about noise around "true" issues. Placing it within the main section of examples means a true issue is displayed directly next to a beginner's section (e.g. 3.12 "Can I use default function parameters with Rcpp?", which then details default parameters).

Recall the entire "corrupting seeds" debacle awhile back... Issues of that magnitude and design choice should be documented somewhere.

Contributor

coatless commented Oct 24, 2016

@eddelbuettel: You know I am going to include that on the PR for the first documented issued.

How would you feel about a subsection within examples? Basically, it's not a moral objection, more so I'm worried about noise around "true" issues. Placing it within the main section of examples means a true issue is displayed directly next to a beginner's section (e.g. 3.12 "Can I use default function parameters with Rcpp?", which then details default parameters).

Recall the entire "corrupting seeds" debacle awhile back... Issues of that magnitude and design choice should be documented somewhere.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Member

Yeah, that is the problem with "single track" listicles. Maybe "Section 5: Known Issues" really is the best we can do.

Member

eddelbuettel commented Oct 24, 2016

Yeah, that is the problem with "single track" listicles. Maybe "Section 5: Known Issues" really is the best we can do.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Oct 24, 2016

Contributor

An alternative is to place known issues in a separate vignette. However, I feel that is just far too much. Perhaps this is better suited for the later API docs I mentioned?

Contributor

coatless commented Oct 24, 2016

An alternative is to place known issues in a separate vignette. However, I feel that is just far too much. Perhaps this is better suited for the later API docs I mentioned?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Member

I thought about that too, and also think it is too far. A FAQ really is the place.

Member

eddelbuettel commented Oct 24, 2016

I thought about that too, and also think it is too far. A FAQ really is the place.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Oct 24, 2016

Contributor

So section 5 it is with a cardinality greater than or equal to one.

Contributor

coatless commented Oct 24, 2016

So section 5 it is with a cardinality greater than or equal to one.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Nov 19, 2016

Contributor

To re-affirm, I have a green light to add a Section 5 still?

Contributor

coatless commented Nov 19, 2016

To re-affirm, I have a green light to add a Section 5 still?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 19, 2016

Member

Did we settle on a Section title?

Member

eddelbuettel commented Nov 19, 2016

Did we settle on a Section title?

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Nov 19, 2016

Contributor

No, contenders were: "Section 5: Known Issues" or "Section 5: Issues"

Contributor

coatless commented Nov 19, 2016

No, contenders were: "Section 5: Known Issues" or "Section 5: Issues"

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 19, 2016

Member

Known Issues is good.

Member

eddelbuettel commented Nov 19, 2016

Known Issues is good.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Mar 23, 2017

Contributor

Second entry for Section: Known Issues

Title: Using \code{operator=} with a scalar replaced the object instead of filling element-wise

Text:

Assignment using the \code{operator=} with either \code{Vector} and \code{Matrix} classes will not illicit an element-wise fill. If you seek an element-wise fill, then use the \code{.fill()} member method to propagate a single value throughout the object. With this being said, the behavior of \code{operator=} differs for the \code{Vector} and \code{Matrix} classes.

The implementation of the \code{operator=} for the \code{Vector} class will replace the existing vector with the assignee value. This behavior is valid even if the assignee value is a scalar value such as 3.14 or 25 as the object is cast into the appropriate \pkg{Rcpp} object type. Therefore, if a \code{Vector} is initialized to have a length of 10 and a scalar is assigned via \code{operator=}, then the resulting \code{Vector} would have a length of 1. See the following code snippet for the aforementioned behavior.

#include<Rcpp.h>

// [[Rcpp::export]]
void vec_scalar_assign(int n, double fill_val) {
  Rcpp::NumericVector X(n);
  Rcpp::Rcout << "Value of Vector on Creation: " << std::endl << X << std::endl;
  X = fill_val;
  Rcpp::Rcout << "Value of Vector after Assignment: " << std::endl << X << std::endl;
}

/*** R
vec_scalar_assign(10, 3.14)
*/

Now, the \code{Matrix} class does not define its own \code{operator=} but instead uses the \code{Vector} class implementation. This leads to unexpected results while attempting to use the assignment operator with a scalar. In particular, the scalar will be coerced into a square \code{Matrix} and then assigned. For an example of this behavior, consider the following code:

#include<Rcpp.h>

// [[Rcpp::export]]
void mat_scalar_assign(int n, double fill_val) { 
  Rcpp::NumericMatrix X(n, n); 
  Rcpp::Rcout << "Value of Matrix on Creation: " << std::endl << X << std::endl;
  X = fill_val; 
  Rcpp::Rcout << "Value of Matrix after Assignment: " << std::endl << X << std::endl;
}

/*** R
mat_scalar_assign(2L, 4)
*/
Contributor

coatless commented Mar 23, 2017

Second entry for Section: Known Issues

Title: Using \code{operator=} with a scalar replaced the object instead of filling element-wise

Text:

Assignment using the \code{operator=} with either \code{Vector} and \code{Matrix} classes will not illicit an element-wise fill. If you seek an element-wise fill, then use the \code{.fill()} member method to propagate a single value throughout the object. With this being said, the behavior of \code{operator=} differs for the \code{Vector} and \code{Matrix} classes.

The implementation of the \code{operator=} for the \code{Vector} class will replace the existing vector with the assignee value. This behavior is valid even if the assignee value is a scalar value such as 3.14 or 25 as the object is cast into the appropriate \pkg{Rcpp} object type. Therefore, if a \code{Vector} is initialized to have a length of 10 and a scalar is assigned via \code{operator=}, then the resulting \code{Vector} would have a length of 1. See the following code snippet for the aforementioned behavior.

#include<Rcpp.h>

// [[Rcpp::export]]
void vec_scalar_assign(int n, double fill_val) {
  Rcpp::NumericVector X(n);
  Rcpp::Rcout << "Value of Vector on Creation: " << std::endl << X << std::endl;
  X = fill_val;
  Rcpp::Rcout << "Value of Vector after Assignment: " << std::endl << X << std::endl;
}

/*** R
vec_scalar_assign(10, 3.14)
*/

Now, the \code{Matrix} class does not define its own \code{operator=} but instead uses the \code{Vector} class implementation. This leads to unexpected results while attempting to use the assignment operator with a scalar. In particular, the scalar will be coerced into a square \code{Matrix} and then assigned. For an example of this behavior, consider the following code:

#include<Rcpp.h>

// [[Rcpp::export]]
void mat_scalar_assign(int n, double fill_val) { 
  Rcpp::NumericMatrix X(n, n); 
  Rcpp::Rcout << "Value of Matrix on Creation: " << std::endl << X << std::endl;
  X = fill_val; 
  Rcpp::Rcout << "Value of Matrix after Assignment: " << std::endl << X << std::endl;
}

/*** R
mat_scalar_assign(2L, 4)
*/
@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Mar 31, 2017

Contributor

@eddelbuettel: This issue can now be closed.

Contributor

coatless commented Mar 31, 2017

@eddelbuettel: This issue can now be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment