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

Use is_unspecified() in vec_cast.list.default() #697

Merged
merged 3 commits into from
Jan 3, 2020

Conversation

DavisVaughan
Copy link
Member

Before:

vctrs::vec_cast(NA, list())
#> [[1]]
#> [1] NA

After:

vctrs::vec_cast(NA, list())
#> [[1]]
#> NULL

We had a test for the list(NA) result, but I'm not sure that's correct?

Open question, what should this return?

vctrs::vec_cast(c(NA, 1), list())
#> [[1]]
#> [1] NA
#> 
#> [[2]]
#> [1] 1

I feel like this should be list(NULL, 1)? It would still be reversible, i.e. vec_cast()ing back to a double would give the original result.

@lionel-
Copy link
Member

lionel- commented Dec 12, 2019

I think we used to consider vec_cast(x, list()) would be equivalent to as.list(), and in this case we'd like to preserve the values of all elements. So the idea was that NA would be converted to NULL for the list-of case but not the list case.

However, nowadays we have vec_chop() for that purpose, which I think is a better primitive:

vec_chop(c(NA, 2, 3))
#> <list_of<double>[3]>
#> [[1]]
#> [1] NA
#>
#> [[2]]
#> [1] 2
#>
#> [[3]]
#> [1] 3

Though it should probably return a bare list rather than a list-of.

@lionel-
Copy link
Member

lionel- commented Dec 12, 2019

It's a bit weird that in the other direction we have two possible missing values, but I guess that's ok. One kind of missing value comes from manipulating the atomic type, and the other kind comes from manipulating the list:

vec_cast(list(NA, NULL, TRUE), logical())
#> [1]   NA   NA TRUE

@DavisVaughan
Copy link
Member Author

DavisVaughan commented Jan 2, 2020

Regarding: #697 (comment)

  • I've made it so that vec_cast(x, list()) replaces NA values in x with NULL in the resulting list.

  • vec_chop() does now return a bare list, so it can be used as the way to preserve the values of all elements.


I've made an additional tweak to ensure that data frames can be cast to shaped lists.

Casting arrays to lists is not going to work right now. Part of that has to do with using x[[i]] and seq_along(x) on the array, which results in the wrong sizes. The other part has to do with vec_equal_na() not working for arrays (#722). It is too much work to fix all of that here.


Also noting that if we had the vec_chop() equivalent for vec_get() (#626), then we could again get rid of vec_cast.list.data.frame() and only have:

vec_cast.list.default <- function(x, to, ...) {
  if (is_unspecified(x)) {
    return(vec_init(to, length(x)))
  }
  
  out <- vec_chop2(x) # `vec_chop()` equivalent for `vec_get()`
  
  vec_slice(out, vec_equal_na(x)) <- list(NULL)
  
  if (!is.object(to)) {
    out <- shape_broadcast(out, to)
  }
  
  out
}

@DavisVaughan DavisVaughan requested a review from lionel- January 2, 2020 15:55
@DavisVaughan DavisVaughan merged commit ae88794 into r-lib:master Jan 3, 2020
@DavisVaughan DavisVaughan deleted the vec-cast-list branch January 3, 2020 14:30
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