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

subsetting an S3 vector fails with multiple arguments #1880

Open
devinrkeane opened this issue Sep 29, 2023 · 5 comments
Open

subsetting an S3 vector fails with multiple arguments #1880

devinrkeane opened this issue Sep 29, 2023 · 5 comments
Labels
bug an unexpected problem or unintended behavior

Comments

@devinrkeane
Copy link

devinrkeane commented Sep 29, 2023

we recently created a custom S3 vector class to handle categorical data and are running into issues whenever this vector encounters base subsetting beyond a single dimension or argument.

Our class is similar to haven's labelled class and I provided an example where this gets us:

  # basic integer subsets fine with mutliple args
  y <- 1:2
  
  y[2, drop = FALSE]
  
# questionr contains this line of code, works fine here
  questionr::wtd.table(y, weights = c(1,1))
  
  # custom integer does not
  x <- haven::labelled(c(1:2), labels = c("apples" = 1L, "oranges" = 2L), label = "lab")
  
  x[2, drop = FALSE]
  
# questionr contains this line of code above and it's breaking things for us in a few areas
  questionr::wtd.table(x, weights = c(1,1))

Is this desired behavior and if so, should we be messaging to the user what the issue is or what we should be avoiding? This seems to be a flaw but i dont have a deep understanding of the vctrs theory, so any guidance here would be appreciated.

We're on 4.1.3, up to date with vctrs (0.6.3).

@devinrkeane devinrkeane changed the title subsetting an S3 vector fails with more than subsetting an S3 vector fails with multiple arguments Sep 29, 2023
@DavisVaughan
Copy link
Member

I think it is a bug on our end.

I think it is because

> vctrs:::`[.vctrs_vctr`
function (x, i, ...) 
{
    vec_index(x, i, ...)
}

doesn't have a named argument for drop in the signature so it ends up in the ... which gets passed on to vec_index()

> vctrs:::vec_index
function (x, i, ...) 
{
    i <- maybe_missing(i, TRUE)
    out <- vec_slice(x, i)
    if (!dots_n(...)) {
        return(out)
    }
    proxy <- vec_data(out)
    out <- proxy[, ..., drop = FALSE]
    vec_restore(out, x)
}

so we end up doing something like this:

> vctrs:::vec_index(1:2, 2, drop = FALSE)
Error in proxy[, ..., drop = FALSE] : incorrect number of dimensions

or, more simply:

> x <- 1:2
> x[, drop = FALSE, drop = FALSE]
Error in x[, drop = FALSE, drop = FALSE] : incorrect number of dimensions

@DavisVaughan DavisVaughan added the bug an unexpected problem or unintended behavior label Sep 29, 2023
@devinrkeane
Copy link
Author

thanks! Makes sense, that's where i ended up too in my debugging,

 x <- haven::labelled(c(1:2), labels = c("apples" = 1L, "oranges" = 2L), label = "lab")
 vctrs:::vec_index(x, 2, drop=FALSE)

@hhhh5
Copy link

hhhh5 commented Sep 10, 2024

Is there a way to patch this on the user's side in the meantime? I used a combination of unlockBinding() and assign() to overwrite [.vctrs_vctr but there might be cleaner solutions.

@nalimilan
Copy link

FWIW, this also creates problem for me. In my lab we use a lot questionr's wtd.table with labelled arrays created by haven and it fails (juba/questionr#137).

@nalimilan
Copy link

@DavisVaughan Do you mean the fix is just that vec_index should take a drop argument, and ignore it (given that it doesn't make a difference for vectors)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

4 participants