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

cbind on tbl_df should return a tbl_df? #779

Closed
daattali opened this issue Nov 18, 2014 · 9 comments
Closed

cbind on tbl_df should return a tbl_df? #779

daattali opened this issue Nov 18, 2014 · 9 comments
Assignees
Labels
Milestone

Comments

@daattali
Copy link
Contributor

@daattali daattali commented Nov 18, 2014

Not sure if this is a feature or a bug - if I cbind two separate tbl_df objects, I expected the result to also be a tbl_df rather than having to cast it myself.

df <- tbl_df(data.frame(a = letters))
DF <- tbl_df(data.frame(A = LETTERS))
bound <- cbind(df, DF)
class(bound)
@romainfrancois
Copy link
Member

@romainfrancois romainfrancois commented Nov 18, 2014

We don't have control over what cbind does. dplyr had bind_all and cbind_list but that also gives a data.frame no matter what. See https://github.com/hadley/dplyr/blob/master/src/bind.cpp#L171

Perhaps we should replace this line :

out.attr( "class" ) = "data.frame" ;

by this line to make a tbl_df instead, as this is what rbind_* are doing.

out.attr( "class" ) = classes_not_grouped() ;

@hadley
Copy link
Member

@hadley hadley commented Nov 18, 2014

@romainfrancois can you please make that change? Ideally we'd preserve the input class, but it's hard to know what to do if you have a mix of tbl df's and data frames

@daattali
Copy link
Contributor Author

@daattali daattali commented Nov 18, 2014

Thanks for responding so promptly.
IMO the safest behaviour would be to revert to the "most recent common ancestor" -- I would expect to get a data.frame back if I pass in a data.frame and a tbl_df. But that's clearly a design choice up to you that I would not argue about.

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 18, 2014

We don't have control over what cbind does.

Actually cbind accepts S3 methods. So it's possible to produce a tbl_df whenever the first argument is a tbl_df. However, we can't have this kind of control if the first argument is a data.frame.

So, for consistency, cbind.tbl_df and friends could produce a tbl_df when all arguments are tbl_dfs, and a data.frame otherwise?

@hadley
Copy link
Member

@hadley hadley commented Nov 18, 2014

@lionelgit have you read the dispatch section in help? It's not standard S3 dispatch, and I'm pretty sure it won't work when you have a mix of tbl dfs and data frames. But I'm happy to be proved wrong

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 18, 2014

Indeed, I didn't know this subtlety.

Still, it may make sense to have a cbind.tbl_df method to produce a tbl_df when all arguments are tbl_df.

@hadley hadley added the feature label Nov 20, 2014
@hadley hadley added this to the 0.4 milestone Nov 20, 2014
@steromano
Copy link

@steromano steromano commented Mar 2, 2015

I'll just comment here instead of opening a new issue, as this is likely a small thing.
Contrary to what the documentation says, cbind_cols seems to always return a data.frame:

x <- as.tbl(iris)
bind_cols(x, x) %>% class  # data.frame

@romainfrancois romainfrancois self-assigned this Mar 3, 2015
@jennybc
Copy link
Member

@jennybc jennybc commented Apr 16, 2015

👍 to this thread and proposed enhancement. What @steromano said: when I supply two tbl_dfs to bind_cols(), I get a data.frame back. It's unexpected and contradicts the documentation.

@romainfrancois
Copy link
Member

@romainfrancois romainfrancois commented Apr 20, 2015

Now it's always making a tbl_df which is harmless anyway, even if both are just "data.frame".

@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
6 participants