Skip to content
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

Allow assignment of size 1 with Subsetter #349

Merged
merged 5 commits into from
Sep 7, 2015

Conversation

fplazaonate
Copy link
Contributor

R allows singleton vectors to be assigned to vectors subsets with more than one element.

Here is an example:

> v1=c(1,2,3)
> v2=c(4)
> v1[v1<4]=v2
> v1
[1] 4 4 4

This feature is currently not available in Rcpp as previously discussed in #345

Code:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
IntegerVector test()
{
  IntegerVector v1 = IntegerVector::create(1,2,3);
  IntegerVector v2 = IntegerVector::create(4);
  v1[v1<4] = v2;

  return v1;
}

/*** R
test()
*/

Result:

test()
Error in eval(expr, envir, enclos) : index error

This PR fixes this issue by returning the same result as R:

test()
[1] 4 4 4

@eddelbuettel
Copy link
Member

Florian, can we please try it the other way around as previously discussed:

  • identify an issue
  • create consensus that a code fix is needed
  • work out a code fix
  • create a pull request
  • ensure the pull request is complete with
    • unit tests
    • documentation
    • ChangeLog entry

This one once again start from the wrong (though I agree that it is of course a very small one...).

@fplazaonate
Copy link
Contributor Author

We briefly discussed this issue here #345 but I will create a new one

@eddelbuettel
Copy link
Member

We can keep this open, it looks like a valid patch. I just do not like "random" pull requests that fly in without prior discussion. In this case, if #345 is/was indeed the place where this was discussed, then the fault is on me for not remembering -- and you for not referencing it. I have to deal with more than one discussion at a time, so helpful pointers are appreciated.

In any event, documentation, tests, changelog entries etc pp are also clearly missing from the PR. So please reopen and augment as you see fit.

@fplazaonate fplazaonate reopened this Aug 27, 2015
@fplazaonate
Copy link
Contributor Author

Could you have a look to my last commits?

@eddelbuettel
Copy link
Member

The patch is fine as far as it mechanics go. But I can't make up my mind on this:

  • on the hand it is nice to have a new subsetter
  • but on the other hand it is a very specific case of recycling
    • what about vectors other than length one?
    • shouldn't an element other than position 0 be assignable?

Recycling has very specific meaning in R. We have avoided it here, mostly, as it is not natural for C++.

I am not yet convinced this really helps.

@kevinushey @jjallaire Any thoughts?

@kevinushey
Copy link
Contributor

Looks good to me! The PR fixes a bug in the old implementation (the goal was to allow recycling in the first place; it just was not implemented correctly)

@eddelbuettel
Copy link
Member

Thanks for that.

@eddelbuettel
Copy link
Member

Ok, with this and the parallel email discussion this is good to go.

eddelbuettel added a commit that referenced this pull request Sep 7, 2015
Allow assignment of size 1 with Subsetter
@eddelbuettel eddelbuettel merged commit 807cf8c into RcppCore:master Sep 7, 2015
@fplazaonate fplazaonate deleted the subsetter-patch branch September 8, 2015 15:33
@coatless coatless mentioned this pull request Jul 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants