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

vec_unique() fails on lists of objects containing non-vectors #643

Closed
mikmart opened this issue Nov 4, 2019 · 6 comments · Fixed by #661
Closed

vec_unique() fails on lists of objects containing non-vectors #643

mikmart opened this issue Nov 4, 2019 · 6 comments · Fixed by #661

Comments

@mikmart
Copy link

@mikmart mikmart commented Nov 4, 2019

In the current dev version, vec_unique() doesn’t work with lists of objects that are not vectors, or that contain non-vector elements in a recursive structure:

m <- glm(mpg ~ wt, data = mtcars)
vctrs::vec_unique(list(m, m))
#> Error: `x` must be a vector, not a `glm/lm` object

o <- list(x = expression(x))
vctrs::vec_unique(list(o, o))
#> Error: `x` must be a vector, not an expression vector

glm objects are an example of both, and can frequently appear in list columns in data frames. As vec_unique() gets more use behind the scenes in packages such as tidyr, this threatens to break code that works with list columns: e.g. tidyverse/tidyr#735 seems to be a symptom of this behaviour.

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

@DavisVaughan

This comment has been minimized.

Copy link
Collaborator

@DavisVaughan DavisVaughan commented Nov 4, 2019

Part of this will be fixed by #634, but I'm not quite happy with that solution yet.

While working on that, I also noticed that vec_equal() (and through this, the dictionary functions which call equal_scalar()) doesn't work correctly with lists containing scalar objects.

vctrs::vec_equal(list(lm(mpg~cyl, data = mtcars)), list(lm(mpg~cyl, data = mtcars)))
#> Error: `x` must be a vector, not a `lm` object

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

This comes from equal_object() calling vec_size().

R_len_t n = vec_size(x);

This is a bit tough because we are using this as the fallthrough for most vector types, and want it to work for data frames too, which is why we use vec_size(). I think we might need to separate out the VECSXP case and use Rf_length() when it is a true list, and vec_size() when it is a data frame.

@lionel-

This comment has been minimized.

Copy link
Member

@lionel- lionel- commented Nov 4, 2019

I think this lm object should be checked with an obj_ function, which should use base R primitives to descend through all objects, including data frames.

@DavisVaughan

This comment has been minimized.

Copy link
Collaborator

@DavisVaughan DavisVaughan commented Nov 4, 2019

Ah I see. A data frame would not have to be checked row wise when it is inside a list like vec_equal(list(mtcars), list(mtcars)) because we just need to return a single TRUE/FALSE. That makes a lot of sense.

I think everything is already set up correctly. equal_object() is being called on the two lm objects (which I think is the obj_ function you want). I think that equal_object() just needs to be rewritten to use the base R primitives.

@hadley

This comment has been minimized.

Copy link
Member

@hadley hadley commented Nov 4, 2019

This is another a case where we should be using inherits(x, "list") rather than typeof(x) == "list".

@lionel-

This comment has been minimized.

Copy link
Member

@lionel- lionel- commented Nov 4, 2019

I don't think so because there is no dispatch involved in these equality checks / hashing.

@DavisVaughan

This comment has been minimized.

Copy link
Collaborator

@DavisVaughan DavisVaughan commented Nov 13, 2019

With the dev version:

m <- glm(mpg ~ wt, data = mtcars)
vctrs::vec_unique(list(m, m))
#> [[1]]
#> 
#> Call:  glm(formula = mpg ~ wt, data = mtcars)
#> 
#> Coefficients:
#> (Intercept)           wt  
#>      37.285       -5.344  
#> 
#> Degrees of Freedom: 31 Total (i.e. Null);  30 Residual
#> Null Deviance:       1126 
#> Residual Deviance: 278.3     AIC: 166

o <- list(x = expression(x))
vctrs::vec_unique(list(o, o))
#> [[1]]
#> [[1]]$x
#> expression(x)

Created on 2019-11-13 by the reprex package (v0.3.0.9000)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.