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

Breakage due to dplyr 1.10.0 #269

Closed
bwiernik opened this issue Jan 31, 2023 · 4 comments · Fixed by #270
Closed

Breakage due to dplyr 1.10.0 #269

bwiernik opened this issue Jan 31, 2023 · 4 comments · Fixed by #270

Comments

@bwiernik
Copy link

dplyr 1.10.0 made many changes under the hood, and this is breaking several different different areas of functionality with posterior:

library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
library(posterior)
#> This is posterior version 1.3.1.9000
#> 
#> Attaching package: 'posterior'
#> The following objects are masked from 'package:stats':
#> 
#>     mad, sd, var
#> The following objects are masked from 'package:base':
#> 
#>     %in%, match

n_param <- 4
dat <- tibble(
  a = rvar(array(rnorm(4000 * n_param, mean = rep(1:n_param, each = 4000), sd = 1), dim = c(4000, n_param)))
)

mutate(dat, b = first(a))
#> Error in `mutate()`:
#> ℹ In argument: `b = first(a)`.
#> Caused by error in `.subset2()`:
#> ! subscript out of bounds

#> Backtrace:
#>      ▆
#>   1. ├─dplyr::mutate(dat, b = first(a))
#>   2. ├─dplyr:::mutate.data.frame(dat, b = first(a))
#>   3. │ └─dplyr:::mutate_cols(.data, dplyr_quosures(...), by)
#>   4. │   ├─base::withCallingHandlers(...)
#>   5. │   └─dplyr:::mutate_col(dots[[i]], data, mask, new_columns)
#>   6. │     └─mask$eval_all_mutate(quo)
#>   7. │       └─dplyr (local) eval()
#>   8. ├─dplyr::first(a)
#>   9. │ └─dplyr::nth(x, 1L, order_by = order_by, default = default, na_rm = na_rm)
#>  10. │   └─dplyr:::vec_slice2(x, n)
#>  11. └─dplyr (local) `<fn>`(`<sbscOOBE>`)
#>  12.   └─rlang::abort(message, class = error_class, parent = parent, call = error_call)


n_param <- 4
n_col <- 3
dat <- tibble(
  a = rvar(array(rnorm(4000 * n_param * n_col, mean = rep(1:n_param, each = 4000), sd = 1), dim = c(4000, n_param, n_col)))
)

dat |> 
  rowwise() |> 
  mutate(b = a[,3])
#> Error in `mutate()`:
#> ℹ In argument: `b = a[, 3]`.
#> ℹ In row 1.
#> Caused by error in `a[, 3]`:
#> ! incorrect number of dimensions

#> Backtrace:
#>      ▆
#>   1. ├─dplyr::mutate(rowwise(dat), b = a[, 3])
#>   2. ├─dplyr:::mutate.data.frame(rowwise(dat), b = a[, 3])
#>   3. │ └─dplyr:::mutate_cols(.data, dplyr_quosures(...), by)
#>   4. │   ├─base::withCallingHandlers(...)
#>   5. │   └─dplyr:::mutate_col(dots[[i]], data, mask, new_columns)
#>   6. │     └─mask$eval_all_mutate(quo)
#>   7. │       └─dplyr (local) eval()
#>   8. └─base::.handleSimpleError(...)
#>   9.   └─dplyr (local) h(simpleError(msg, call))
#>  10.     └─rlang::abort(message, class = error_class, parent = parent, call = error_call)

Created on 2023-01-31 with reprex v2.0.2

Both of these worked correctly with dplyr 1.9

@mjskay
Copy link
Collaborator

mjskay commented Jan 31, 2023

Thanks @bwiernik for spotting this. Fortunately I think this might be an easy fix (and one that should incidentally also close #267 ). Basically vctrs::vec_is_list() should be FALSE for rvars, and isn't, because we include "list" in the class definition. I had thought it was necessary, but removing it does not seem to break anything (might need to double check on that...).

@bwiernik if you have a chance, can you check and see if you have any other breakages if you install this branch: remotes::install_github("stan-dev/posterior@new_dplyr")? Thanks!

@bwiernik
Copy link
Author

bwiernik commented Feb 1, 2023

yep, all of my functions are working fine with that branch

@paul-buerkner
Copy link
Collaborator

Feel free to merge @mjskay

@mjskay
Copy link
Collaborator

mjskay commented Feb 1, 2023

Thanks @bwiernik and @paul-buerkner! I'll merge this later today or tomorrow once I do a few more checks.

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 a pull request may close this issue.

3 participants