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

Arithmetic unexpectedly works with base S3 classes #1853

Open
wurli opened this issue Jun 28, 2023 · 2 comments
Open

Arithmetic unexpectedly works with base S3 classes #1853

wurli opened this issue Jun 28, 2023 · 2 comments

Comments

@wurli
Copy link

wurli commented Jun 28, 2023

Working through 'Arithmetic' in vignette("s3-vector", "vctrs") to create my own class, I've hit some undesirable behaviour and I'm struggling to see how to address it. My issue is that arithmetic with my class and a base-R class actually works when I'd prefer it to throw an error. This behaviour manifests with the <vctrs_meter> class defined in the vignette, so I've used this class as a reprex below.

library(vctrs)
#> Warning: package 'vctrs' was built under R version 4.2.3

# -- Define a <vctrs_meter> class as per vignette ---------------------------------------------------------

meter <- function(x) {
  x <- vec_cast(x, double())
  new_meter(x)
}
new_meter <- function(x) {
  stopifnot(is.double(x))
  new_vctr(x, class = "vctrs_meter")
}
format.vctrs_meter <- function(x, ...) {
  paste0(format(vec_data(x)), " m")
}
vec_arith.vctrs_meter <- function(op, x, y, ...) {
  UseMethod("vec_arith.vctrs_meter", y)
}
vec_arith.vctrs_meter.default <- function(op, x, y, ...) {
  stop_incompatible_op(op, x, y)
}
vec_arith.vctrs_meter.vctrs_meter <- function(op, x, y, ...) {
  switch(
    op,
    "+" = ,
    "-" = new_meter(vec_arith_base(op, x, y)),
    "/" = vec_arith_base(op, x, y),
    stop_incompatible_op(op, x, y)
  )
}

# -- Expected behaviour with built-in types ---------------------------------------------------------------

# This works as expected
meter(10) + meter(1)
#> <vctrs_meter[1]>
#> [1] 11 m

# This doesn't work, also as expected
meter(10) + 1L
#> Error in `vec_arith()`:
#> ! <vctrs_meter> + <integer> is not permitted
#> Backtrace:
#>     ▆
#>  1. └─vctrs:::`+.vctrs_vctr`(meter(10), 1L)
#>  2.   ├─vctrs::vec_arith("+", e1, e2)
#>  3.   ├─global vec_arith.vctrs_meter("+", e1, e2)
#>  4.   └─global vec_arith.vctrs_meter.default("+", e1, e2)
#>  5.     └─vctrs::stop_incompatible_op(op, x, y)
#>  6.       └─vctrs:::stop_incompatible(...)
#>  7.         └─vctrs:::stop_vctrs(...)
#>  8.           └─rlang::abort(message, class = c(class, "vctrs_error"), ..., call = call)

# -- Unexpected behaviour with base S3 classes ------------------------------------------------------------

# These all work with warnings, whereas I'd expect an error
meter(1) + factor("hi")
#> Warning: Incompatible methods ("+.vctrs_vctr", "Ops.factor") for "+"
#> <vctrs_meter[1]>
#> [1] 2 m
meter(1) + Sys.Date()
#> Warning: Incompatible methods ("+.vctrs_vctr", "+.Date") for "+"
#> <vctrs_meter[1]>
#> [1] 19537 m
meter(1) + as.POSIXct(Sys.Date())
#> Warning: Incompatible methods ("+.vctrs_vctr", "+.POSIXt") for "+"
#> <vctrs_meter[1]>
#> [1] 1687910401 m

Created on 2023-06-28 with reprex v2.0.2

I'm aware that this is probably intended behaviour, but I'm creating an issue because I think this problem and the intended solution should be outlined in the vignette.

@wurli wurli changed the title Arithmetic works with more classes than expected Arithmetic unexpectedly works with base S3 classes Jun 28, 2023
@TimTaylor
Copy link

I think tweaking the documentation would be helpful here. The problem is all of those classes (Date, POSIXt and factor) have their own default Ops methods (hence the incompatible call). As of R 4.3.0 you could use the chooseOpsMethod generic (to work around this) but this is very new and I'm not sure the {vctrs} developers have yet thought about how it ties in with the vctrs framework (from what I can tell anyway).

@wurli
Copy link
Author

wurli commented Jun 29, 2023

Thanks, I've been using this approach and it seems to do the trick, although I needed to upgrade my version of R. Still interested to hear from developers though, especially regarding whether this will be the advised solution in future, or whether it'll get baked into vctrs itself.

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

No branches or pull requests

2 participants