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

has_rownames() glitch #852

Closed
romainfrancois opened this issue Feb 11, 2021 · 3 comments · Fixed by #862
Closed

has_rownames() glitch #852

romainfrancois opened this issue Feb 11, 2021 · 3 comments · Fixed by #862
Milestone

Comments

@romainfrancois
Copy link
Member

I believe has_rownames() suffers from a problem in .row_names_info(type = 1):

df <- data.frame(x = 1:10)
df2 <- structure(df, .drop = FALSE)
df3 <- `attr<-`(df, ".drop", FALSE)

.row_names_info(df, type = 0)
#> [1]  NA -10
.row_names_info(df2, type = 0)
#> [1] NA 10
.row_names_info(df3, type = 0)
#> [1]  NA -10

.row_names_info(df, type = 1)
#> [1] -10
.row_names_info(df2, type = 1)
#> [1] 10
.row_names_info(df3, type = 1)
#> [1] -10

.row_names_info(df, type = 2)
#> [1] 10
.row_names_info(df2, type = 2)
#> [1] 10
.row_names_info(df3, type = 2)
#> [1] 10

Created on 2021-02-11 by the reprex package (v0.3.0)

Specifically:

.row_names_info(df2, type = 1)
#> [1] 10

In the end, I believe this is a bug in structure(), and I've added a fix to dplyr so that it does not use structure() in the problem from tidyverse/dplyr#5745 but I still believe has_rownames() could work around that quirk.

@krlmlr
Copy link
Member

krlmlr commented Feb 12, 2021

Thanks. I think the problem really is in structure(), though it might be difficult to fix (or even work as documented, haven't checked).

I also remember this worked differently in older versions of R. With current R 4.0, we could indeed tweak the detection of this case and check the class of .row_names_info() instead:

df <- data.frame(x = 1:10)
df2 <- structure(df, .drop = FALSE)

dput(df)
#> structure(list(x = 1:10), class = "data.frame", row.names = c(NA, 
#> -10L))
dput(df2)
#> structure(list(x = 1:10), class = "data.frame", row.names = c(NA, 
#> 10L), .drop = FALSE)

identical(row.names(df), row.names(df2))
#> [1] TRUE

.row_names_info(df, 0)
#> [1]  NA -10
.row_names_info(df2, 0)
#> [1] NA 10
.row_names_info(mtcars, 0)
#>  [1] "Mazda RX4"           "Mazda RX4 Wag"       "Datsun 710"         
#>  [4] "Hornet 4 Drive"      "Hornet Sportabout"   "Valiant"            
#>  [7] "Duster 360"          "Merc 240D"           "Merc 230"           
#> [10] "Merc 280"            "Merc 280C"           "Merc 450SE"         
#> [13] "Merc 450SL"          "Merc 450SLC"         "Cadillac Fleetwood" 
#> [16] "Lincoln Continental" "Chrysler Imperial"   "Fiat 128"           
#> [19] "Honda Civic"         "Toyota Corolla"      "Toyota Corona"      
#> [22] "Dodge Challenger"    "AMC Javelin"         "Camaro Z28"         
#> [25] "Pontiac Firebird"    "Fiat X1-9"           "Porsche 914-2"      
#> [28] "Lotus Europa"        "Ford Pantera L"      "Ferrari Dino"       
#> [31] "Maserati Bora"       "Volvo 142E"

Created on 2021-02-12 by the reprex package (v1.0.0)

Need to review what the behavior looks like in R 3.3 .

@romainfrancois
Copy link
Member Author

Yeah, the underlying problem is here: https://github.com/wch/r-source/blob/b30641d3f58703bbeafee101f983b6b263b7f27d/src/main/attrib.c#L71

Maybe it will be picked up eventually in R, but in the meantime, tibble could be defensive.

@krlmlr krlmlr added this to the 3.1.0 milestone Feb 14, 2021
krlmlr added a commit that referenced this issue Feb 23, 2021
- `has_rownames()` now works correctly for data frames with a `"row.names"` attribute malformed due to a problem in `structure()` (#852).
@github-actions
Copy link
Contributor

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 Feb 24, 2022
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