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

rbind_all should raise exception if arguments aren't data.frames #288

Closed
coolbutuseless opened this issue Feb 27, 2014 · 5 comments
Closed

rbind_all should raise exception if arguments aren't data.frames #288

coolbutuseless opened this issue Feb 27, 2014 · 5 comments
Assignees
Labels
Milestone

Comments

@coolbutuseless
Copy link
Contributor

@coolbutuseless coolbutuseless commented Feb 27, 2014

I mistakenly tried to rbind_all() on a list of vectors, and did not get what I was expecting.

rbind_all() only seems to work with data.frames (as with the rest of dplyr)

Perhaps raise an exception in the case where arguments aren't data.frames? Or is the result I go expected somehow?

e.g.

> ll <- list(c(1,2,3,4, 5), c(6, 7, 8, 9, 10))
> do.call(rbind, ll)
     [,1] [,2] [,3] [,4] [,5]
[1,]    1    2    3    4    5
[2,]    6    7    8    9   10
> rbind_all(ll)
   c(1, 2, 3, 4, 5) c(6, 7, 8, 9, 10)
1                 1                NA
2                 2                NA
3                 3                NA
4                 4                NA
5                 5                NA
6                NA                 6
7                NA                 7
8                NA                 8
9                NA                 9
10               NA                10
> 
@romainfrancois
Copy link
Member

@romainfrancois romainfrancois commented Feb 27, 2014

What happens internally is a call to as.data.frame because I'm using a ListOf<DataFrame>. and:

> lapply( ll, as.data.frame )
[[1]]
  X[[1L]]
1       1
2       2
3       3
4       4
5       5

[[2]]
  X[[2L]]
1       6
2       7
3       8
4       9
5      10

Perhaps we can be stricter and indeed only allow data frames in. Although I've seen example of code using matrices as arguments to rbind_*

@hadley hadley added this to the v0.2 milestone Mar 17, 2014
@hadley
Copy link
Member

@hadley hadley commented Mar 17, 2014

@romainfrancois I think in generally rbind_all should be pretty strict, and only accept data frames. We might later add a generic version that is more flexible.

@romainfrancois
Copy link
Member

@romainfrancois romainfrancois commented Mar 17, 2014

Done with the addition of the StrictListOf class template. Maybe there is space for a better error message:

> ll <- list(c(1,2,3,4, 5), c(6, 7, 8, 9, 10))
> rbind_all(ll)
Error: object at index 0 not compatible with class Rcpp::DataFrame_Impl<Rcpp::PreserveStorage>

@hadley
Copy link
Member

@hadley hadley commented Mar 18, 2014

Could we make the error message: "Object at index 1 not a data.frame"?

Also I think this broken NULL handling: https://travis-ci.org/hadley/dplyr/jobs/21027956#L1781

@romainfrancois
Copy link
Member

@romainfrancois romainfrancois commented Mar 18, 2014

I'll make the error message 1-index-based and more R friendly.

About the NULL handling, I forgot this requirement. So we ignore NULL, but otherwise we only accept data frames. I'll update in the morning.

romainfrancois added a commit that referenced this issue Mar 19, 2014
hadley added a commit that referenced this issue Mar 19, 2014
romainfrancois added a commit that referenced this issue Mar 19, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jun 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants