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

Unexpected error in as_tibble() when rownames = is passed with implicit row names #567

Closed
stufield opened this issue Jan 28, 2019 · 11 comments · Fixed by #678
Closed

Unexpected error in as_tibble() when rownames = is passed with implicit row names #567

stufield opened this issue Jan 28, 2019 · 11 comments · Fixed by #678

Comments

@stufield
Copy link
Contributor

stufield commented Jan 28, 2019

The inconsistency arises in tibble v2.0.1 under the (I think) new arguments passed to as_tibble(), specifically when you wish to create a new "id" column from the existing row names of the object you are coercing. If that object, say a data frame, has default numeric character strings, as_tibble() throws an error saying that the object does not have rownames (but it does!). Additionally, if you explicitly add the rownames, this error is not thrown. Please see the reprex below:

library(tibble)
packageVersion("tibble")
#> [1] '2.0.1'
library(magrittr)
df <- data.frame(V = letters[1:5])
df
#>   V
#> 1 a
#> 2 b
#> 3 c
#> 4 d
#> 5 e
rownames(df)
#> [1] "1" "2" "3" "4" "5"

# Error thrown
df %>% as_tibble(rownames = "num_char")
#> Error: Object passed to `as_tibble()` must have row names if the `rownames` argument is set.

# If you explicetly generate rownames for `df`
# It `as_tibble() proceeds as expected
rownames(df) <- as.character(1:5)
rownames(df)
#> [1] "1" "2" "3" "4" "5"

# No error thrown
df %>% as_tibble(rownames = "num_char")
#> # A tibble: 5 x 2
#>   num_char V    
#>   <chr>    <fct>
#> 1 1        a    
#> 2 2        b    
#> 3 3        c    
#> 4 4        d    
#> 5 5        e

Created on 2019-01-28 by the reprex package (v0.2.1)

Session info
devtools::session_info()
#> ─ Session info ──────────────────────────────────────────────────────────
#>  setting  value                       
#>  version  R version 3.5.2 (2018-12-20)
#>  os       macOS Mojave 10.14.3        
#>  system   x86_64, darwin15.6.0        
#>  ui       X11                         
#>  language (EN)                        
#>  collate  en_US.UTF-8                 
#>  ctype    en_US.UTF-8                 
#>  tz       America/Denver              
#>  date     2019-01-28                  
#> 
#> ─ Packages ──────────────────────────────────────────────────────────────
#>  package     * version date       lib source        
#>  assertthat    0.2.0   2017-04-11 [1] CRAN (R 3.5.0)
#>  backports     1.1.3   2018-12-14 [1] CRAN (R 3.5.0)
#>  callr         3.1.1   2018-12-21 [1] CRAN (R 3.5.0)
#>  cli           1.0.1   2018-09-25 [1] CRAN (R 3.5.0)
#>  crayon        1.3.4   2017-09-16 [1] CRAN (R 3.5.0)
#>  desc          1.2.0   2018-05-01 [1] CRAN (R 3.5.0)
#>  devtools      2.0.1   2018-10-26 [1] CRAN (R 3.5.1)
#>  digest        0.6.18  2018-10-10 [1] CRAN (R 3.5.0)
#>  evaluate      0.12    2018-10-09 [1] CRAN (R 3.5.0)
#>  fansi         0.4.0   2018-10-05 [1] CRAN (R 3.5.0)
#>  fs            1.2.6   2018-08-23 [1] CRAN (R 3.5.0)
#>  glue          1.3.0   2018-07-17 [1] CRAN (R 3.5.0)
#>  highr         0.7     2018-06-09 [1] CRAN (R 3.5.0)
#>  htmltools     0.3.6   2017-04-28 [1] CRAN (R 3.5.0)
#>  knitr         1.21    2018-12-10 [1] CRAN (R 3.5.1)
#>  magrittr    * 1.5     2014-11-22 [1] CRAN (R 3.5.0)
#>  memoise       1.1.0   2017-04-21 [1] CRAN (R 3.5.0)
#>  pillar        1.3.1   2018-12-15 [1] CRAN (R 3.5.0)
#>  pkgbuild      1.0.2   2018-10-16 [1] CRAN (R 3.5.0)
#>  pkgconfig     2.0.2   2018-08-16 [1] CRAN (R 3.5.0)
#>  pkgload       1.0.2   2018-10-29 [1] CRAN (R 3.5.0)
#>  prettyunits   1.0.2   2015-07-13 [1] CRAN (R 3.5.0)
#>  processx      3.2.1   2018-12-05 [1] CRAN (R 3.5.0)
#>  ps            1.3.0   2018-12-21 [1] CRAN (R 3.5.0)
#>  R6            2.3.0   2018-10-04 [1] CRAN (R 3.5.0)
#>  Rcpp          1.0.0   2018-11-07 [1] CRAN (R 3.5.0)
#>  remotes       2.0.2   2018-10-30 [1] CRAN (R 3.5.0)
#>  rlang         0.3.1   2019-01-08 [1] CRAN (R 3.5.2)
#>  rmarkdown     1.11    2018-12-08 [1] CRAN (R 3.5.0)
#>  rprojroot     1.3-2   2018-01-03 [1] CRAN (R 3.5.0)
#>  sessioninfo   1.1.1   2018-11-05 [1] CRAN (R 3.5.0)
#>  stringi       1.2.4   2018-07-20 [1] CRAN (R 3.5.0)
#>  stringr       1.3.1   2018-05-10 [1] CRAN (R 3.5.0)
#>  testthat      2.0.1   2018-10-13 [1] CRAN (R 3.5.0)
#>  tibble      * 2.0.1   2019-01-12 [1] CRAN (R 3.5.2)
#>  usethis       1.4.0   2018-08-14 [1] CRAN (R 3.5.0)
#>  utf8          1.1.4   2018-05-24 [1] CRAN (R 3.5.0)
#>  withr         2.1.2   2018-03-15 [1] CRAN (R 3.5.0)
#>  xfun          0.4     2018-10-23 [1] CRAN (R 3.5.0)
#>  yaml          2.2.0   2018-07-25 [1] CRAN (R 3.5.0)
#> 
#> [1] /Users/sfield/r_libs
#> [2] /Library/Frameworks/R.framework/Versions/3.5/Resources/library
@krlmlr
Copy link
Member

krlmlr commented Jan 28, 2019

Thanks. Looks like this has been added in #388, the old behavior was plain wrong.

In your example, df doesn't have explicit row names, they are created on request. I don't mind changing the behavior to use implicit row names in this situation, would you like to do a PR (against the r-2.0 branch please)?

@stufield
Copy link
Contributor Author

stufield commented Jan 29, 2019

I think I understand. This error makes more sense for as_tibble.matrix() when there are no rownames, but I agree with you that data frames are different. From #388, it looks like the problem might be that as_tibble.matrix() coerces to a data frame and then calls as_tibble.data.frame().

Happy to try a PR, but to clarify, you mean you want me to create my issue-branch based on the branch r-2.0? Or is it all about where I push to when creating the PR?

@krlmlr
Copy link
Member

krlmlr commented Jan 29, 2019

Thanks. Please base off of the r-2.0 branch and also submit the PR against that branch.

@stufield
Copy link
Contributor Author

stufield commented Jan 30, 2019

I'm ready to submit, but left some commented lines in this commit in case we need some back and forth prior to merging. If you agree with my fixes, I will clean up and push again, ok?

@stufield
Copy link
Contributor Author

stufield commented Feb 24, 2019

Hi. Which branch would you like me to base from and PR against? It appears the the r-2.0 branch is now stale (deleted?). So I think it's easiest if I start from scratch, re-fork, and re-submit. Just let me know your preference for the home branch. Thanks!

@krlmlr
Copy link
Member

krlmlr commented Feb 24, 2019

Thanks. I've moved to master for now, r-2.0 is dead. Sorry for the confusion, working on an unplanned intermediate release.

@stufield
Copy link
Contributor Author

stufield commented Feb 24, 2019

No problem. Shall I branch off master and submit a PR to it or hold off altogether until after the unplanned release? Just let me know.

@krlmlr
Copy link
Member

krlmlr commented Nov 1, 2019

This fell through the cracks. Would you still like to submit a PR?

@stufield
Copy link
Contributor Author

stufield commented Nov 4, 2019

Sure I'd be happy to. Just gimme some time to remember what this was all about, re-fork the upstream repo, etc. Still want me to branch off master?

@krlmlr
Copy link
Member

krlmlr commented Nov 4, 2019

Thanks, master is the right branch.

@stufield stufield changed the title Unexpected or inconsistent error in as_tibble() when rownames = is passed with default rownames Unexpected error in as_tibble() when rownames = is passed with implicit row names Nov 19, 2019
stufield added a commit to stufield/tibble that referenced this issue Nov 19, 2019
- logic for implicit rownames in 'as_tibble.data.frame()'
- rm 'error_as_tibble_needs_rownames()' helper
- update 'errors.txt'
- update unit tests; no more rowname errors, add implicit rownames tests
- rm 'error_as_tibble_needs_rownames()' from 'test-msg.R'
- fixes tidyverse#567
stufield added a commit to stufield/tibble that referenced this issue Dec 3, 2019
- logic for implicit rownames in 'as_tibble.data.frame()'
- rm 'error_as_tibble_needs_rownames()' helper
- update 'errors.txt'
- update unit tests; no more rowname errors, add implicit rownames tests
- rm 'error_as_tibble_needs_rownames()' from 'test-msg.R'
- fixes tidyverse#567
krlmlr added a commit that referenced this issue Dec 3, 2019
- `as_tibble.data.frame()` uses implicit row names if asked to create a column from row names. This allows lossless direct conversion of matrices with row names to tibbles (#567, @stufield).
@github-actions
Copy link

github-actions bot commented Dec 8, 2020

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants