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

Implement vec_chop2() #1226

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Implement vec_chop2() #1226

wants to merge 8 commits into from

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Aug 19, 2020

Add vec_chop2() which is the [[ equivalent of vec_chop(). It is still unexported for now because we probably don't want to rush the "2" API but it will be useful internally for implementing vec_map().

With atomic vectors it always transforms inner names into outer names:

vec_chop2(c(foo = 1, bar = 2))
#> $foo
#> [1] 1
#>
#> $bar
#> [1] 2

df <- data.frame(x = 1:2, row.names = c("foo", "bar"))
vec_chop2(df)
#> $foo
#>   x
#> 1 1
#>
#> $bar
#>   x
#> 1 2

mat <- matrix(1:4, 2, dimnames = list(c("foo", "bar"), c("x", "y")))
vec_chop2(mat)
#> $foo
#>      x y
#> [1,] 1 3
#>
#> $bar
#>      x y
#> [1,] 2 4

Since we now require lists to have list storage, it just unstructures them:

x <- list(x = 1, y = 2)
vec_chop2(new_vctr(x))
#> $x
#> [1] 1
#>
#> $y
#> [1] 2

Having this storage requirement really simplifies working with lists. We won't be able to support S4 lists like S4Vectors::List() without introducing something like list_proxy() and list_unproxy() though.

To make the implementation generic, vctrs_rcrd now supports assigning NULL names. This also fixes a bug as names<- would previously be applied to the fields. I still plan to add names support to rcrd vectors in the future but setting names to a character vector is an error for now.

There is a new helper class for unit tests which is a record list that caches the sizes of its elements in attributes.

Copy link
Member

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me! Do we care about indices? Maybe not right now? They are less useful than in vec-chop, I think.

UNPROTECT(1);
} else {
// Zap inner names of atomic vectors. They become outer names.
out = PROTECT(vec_set_names(x, R_NilValue));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can modify x in place if it isn’t referenced? Maybe capture the vec_names earlier to prevent issues if that is the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah

@lionel-
Copy link
Member Author

lionel- commented Aug 19, 2020

Right we can probably wait until there is a need for indices.

@lionel-
Copy link
Member Author

lionel- commented Aug 26, 2020

Having this storage requirement really simplifies working with lists. We won't be able to support S4 lists like S4Vectors::List() without introducing something like list_proxy() and list_unproxy() though.

Ah there's a feature request about this: #1164.

DavisVaughan added a commit to DavisVaughan/tidyr that referenced this pull request Nov 4, 2021
Since we don't have `vec_chop2()` yet r-lib/vctrs#1226
DavisVaughan added a commit to DavisVaughan/tidyr that referenced this pull request Nov 15, 2021
Since we don't have `vec_chop2()` yet r-lib/vctrs#1226
DavisVaughan added a commit to tidyverse/tidyr that referenced this pull request Nov 15, 2021
* Implement `tidyr_chop2()`

Since we don't have `vec_chop2()` yet r-lib/vctrs#1226

* Add `list_of_ptype()` helper for consistent list-of ptype extraction

* Generalize `unchop_col_info()` into `list_init_empty()`

This will be useful for an enhanced `simplify_col()`

* Rename `tidyr_chop2()` to `tidyr_chop()`

* Reimplement rectangling functions

* NEWS bullet

* Move details about `ptype` and `transform` to the `transform` param docs

* Link to `list_of()` documentation

* Mention recycling of `unnest_longer()`

* Synchronize `values_to` and `indices_to` documentation

* Replace "can't" with "must not"

* Introduce `tidyr_temporary_new_list_of()`

Because vctr objects can't currently have `""` names

* Provide more input validation for `ptype` and `transform`

* Add two tests about `names_sep`, provided by @mgirlich

* Chop non-lists into lists with `vec_chop()`

This uses more explainable vctrs tooling for converting non-primary data types (i.e. non-lists) into lists. This also seems to produce the expected output in more scenarios.

Also inlined `tidyr_chop()` into `elt_to_wide()` since that is the only other place it was used. The fact that this removed a helper makes me optimistic that it is the right approach.

* Test that df-cols result in list-ofs when `simplify = FALSE`

* Apply unique name repair on names before applying `names_sep`

Applying it before `names_sep` rather than after means that `""` and `NA_character_` names get repaired early on before they are combined with the prefix and `names_sep`, which can make them mistakently look like "valid" names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants