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

tibble() is dropping inner names #630

Closed
jennybc opened this issue Aug 7, 2019 · 9 comments
Closed

tibble() is dropping inner names #630

jennybc opened this issue Aug 7, 2019 · 9 comments
Labels
vctrs ↗️ Requires vctrs package
Milestone

Comments

@jennybc
Copy link
Member

jennybc commented Aug 7, 2019

Noticed because an incidental update of my local tibble caused pretty catastrophic test failure in tidyr, whose release / revdeps I'm working on. I'm at current HEAD of tibble here.

devtools::load_all("~/rrr/tibble")
#> Loading tibble

df <- tibble(x = 1:2, y = list(a = 1, b = 1:2))
df$y
#> [[1]]
#> [1] 1
#> 
#> [[2]]
#> [1] 1 2
names(df$y)
#> NULL

Created on 2019-08-07 by the reprex package (v0.3.0.9000)

@jennybc
Copy link
Member Author

jennybc commented Aug 7, 2019

This is the guilty commit: defe1ee

@jennybc
Copy link
Member Author

jennybc commented Aug 7, 2019

From the just-previous commit 96cbc7a, I see:

devtools::load_all("~/rrr/tibble")
#> Loading tibble

df <- tibble(x = 1:2, y = list(a = 1, b = 1:2))
df$y
#> $a
#> [1] 1
#> 
#> $b
#> [1] 1 2
names(df$y)
#> [1] "a" "b"

Created on 2019-08-07 by the reprex package (v0.3.0.9000)

@krlmlr
Copy link
Member

krlmlr commented Aug 8, 2019

Thanks. I submitted a PR to fix the tests in tidyr.

Are there legitimate use cases for inner names apart from the (now deprecated) use of .id in unnest()?

This is what I see:

# Names are dropped for atomic vectors
df <- data.frame(a = c(x = 1, y = 2), b = c(z = 3, w = 4))
df$a
#> [1] 1 2
df$b
#> [1] 3 4

# Names are retained for lists
df <- data.frame(a = I(list(x = 1, y = 2)), b = I(list(z = 3, w = 4)))
df$a
#> $x
#> [1] 1
#> 
#> $y
#> [1] 2
df$b
#> $z
#> [1] 3
#> 
#> $w
#> [1] 4

library(tidyverse)

# Names are dropped after dplyr operations
df %>%
  slice(1) %>%
  pull()
#> [[1]]
#> [1] 3

Created on 2019-08-08 by the reprex package (v0.3.0)

It seems that inner names aren't well supported in the tidyverse anyway (nor in base). I'm in favor of removing them early on.

@hadley
Copy link
Member

hadley commented Aug 8, 2019

I don't understand why we are stripping names from all columns. I think you should remove

x[] <- map(x, strip_names)

all together.

@krlmlr
Copy link
Member

krlmlr commented Aug 8, 2019

I don't understand why we should allow names in columns. Are there legitimate use cases?

We never allowed names for columns of atomic types. Now we also drop names from bare lists, via vctrs:::set_names2() (which needs a better solution).

@krlmlr
Copy link
Member

krlmlr commented Aug 23, 2019

  • We'll keep supporting names for list columns, because removing this is likely to cause downstream pain

  • It's not tibble's responsibility to police user input

  • Why do we strip names from atomic vectors? <-- review history

@krlmlr krlmlr added the vctrs ↗️ Requires vctrs package label Oct 27, 2019
@krlmlr
Copy link
Member

krlmlr commented Oct 27, 2019

tibble 1.0 keeps internal names also for atomic types, 1.1 doesn't. Need to bisect.

@krlmlr
Copy link
Member

krlmlr commented Nov 1, 2019

09a07b5 is the first commit that strips internal names from atomic types. It looks like the names were stripped accidentally.

@krlmlr krlmlr closed this as completed in 1be2f3f Nov 1, 2019
@krlmlr krlmlr added this to the 3.0.0 milestone Mar 21, 2020
@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 Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
vctrs ↗️ Requires vctrs package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants