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

base_ops are empty - arithmetic fails e.g. when extending class_double #320

Closed
mmaechler opened this issue Aug 18, 2023 · 7 comments · Fixed by #382
Closed

base_ops are empty - arithmetic fails e.g. when extending class_double #320

mmaechler opened this issue Aug 18, 2023 · 7 comments · Fixed by #382
Milestone

Comments

@mmaechler
Copy link
Collaborator

mmaechler commented Aug 18, 2023

A simple class & object x extending double does work in sin(x) but not with x + 1 or x < 3, because the (internal) base_ops all have no methods defined.

It is easiest to see in an example {latest version of S7}:

> nvec <- new_class("nvec", class_double)
> (x <- nvec(c(1:3, pi, 4:7)))
<nvec> num [1:8] 1 2 3 3.14 4 ...
> sin(x)
<nvec> num [1:8] 8.41e-01 9.09e-01 1.41e-01 1.22e-16 -7.57e-01 ...
> sin(x) == sin(as.double(x))
Error: Can't find method for generic `==(x, y)`:
- x: <nvec>
- y: <double>

Via traceback() we are quickly lead that it is caused by the S7-internal base_ops being "empty", i.e., not having any methods defined.

> traceback()
5: stop(msg, call. = FALSE)
4: function (name, args, signatures) 
   ................
   ................
       list(c("nvec", "double", "S7_object"), "double"))
3: S7::S7_dispatch()
2: base_ops[[.Generic]](e1, e2)
1: Ops.S7_object(sin(x), sin(as.double(x)))

> S7:::base_ops$`==`
<S7_generic> ==(x, y, ...) with 0 methods:

whereas evidently sin() or sinpi() etc are all defined correctly.

@hadley
Copy link
Member

hadley commented Aug 18, 2023

What would you expect the default method to do?

@mmaechler
Copy link
Collaborator Author

The same it does e.g. for sin() {as I wrote already}: Treat its "interior" the same as "double" and do the computation;

  • as S3 and S4 do in this case;
  • as (probably all) the group generic "Math" functions do.

@hadley
Copy link
Member

hadley commented Aug 21, 2023

Should the result still have an S7 class? I assume not.

@mmaechler
Copy link
Collaborator Author

Yes, I think it should, when we are talking about the "default" method: It does keep the class in S3 and S4, too, when combined with a simple numeric

> structure(pi, class="num") + 1
[1] 4.141593
attr(,"class")
[1] "num"
> 1 + structure(pi, class="num")
[1] 4.141593
attr(,"class")
[1] "num"
> 
> setClass("num", contains="numeric");  pi4 <- new("num", pi);   pi4 + 0;   0 + pi4
An object of class "num"
[1] 3.141593
An object of class "num"
[1] 3.141593

Interestingly, that changes when the other vector is of different length, in the same way for S3 and S4:

pi4 + new("num", 0:2)
An object of class "num"
[1] 3.141593 4.141593 5.141593
> pi4 + 0:2  # *not* "num" anymore:
[1] 3.141593 4.141593 5.141593

and ditto for S3:

> structure(pi, class="num") + 0:2
[1] 3.141593 4.141593 5.141593
> structure(pi, class="num") + structure(0:2, class="num")
[1] 3.141593 4.141593 5.141593
attr(,"class")
[1] "num"
> 

@hadley
Copy link
Member

hadley commented Aug 23, 2023

I think this is something that's worth more discussion in a call. It's not clear to me that preserving the existing class (and properties) is the right default because the transformation might break expected relationships between the data and the attributes. There's some question of whether we want to provide a default implementation that's sometimes wrong (forcing the implementor to carefully consider all existing generics and whether the behaviour is correct) or never provide a default so that the implementor is forced to.

Either way, we should consider this with the behaviour for generics like sin() and what the consequences will be for backward compatibility. (IOTW do we need to provide methods for the Math, Summary, and Complex group generics?).

@hadley
Copy link
Member

hadley commented Oct 13, 2023

I think this puts all the current inconsistencies in one place:

library(S7)

foo_S3 <- structure(1, class = "foo") 
class(foo_S3 + 1)
#> [1] "foo"
class(foo_S3 + c(1, 2))
#> [1] "numeric"
class(sin(foo_S3))
#> [1] "foo"

foo_S7 <- new_class("foo", parent = class_double)(1)
class(foo_S7 + 1)
#> Error: Can't find method for generic `+(e1, e2)`:
#> - e1: <foo>
#> - e2: <double>
class(foo_S7 + c(1, 2))
#> Error: Can't find method for generic `+(e1, e2)`:
#> - e1: <foo>
#> - e2: <double>
class(sin(foo_S7))
#> [1] "foo"       "double"    "S7_object"

Created on 2023-10-13 with reprex v2.0.2

@hadley
Copy link
Member

hadley commented Oct 16, 2023

Probably easiest to argue that it should be consistent with S3. You are inheriting from an base type so that you are choosing the chaos. If you don't want the chaos then:

  • Use composition instead of inheritance
  • Provide methods for Math, Ops, etc.

@hadley hadley added this to the v0.2.0 milestone Oct 17, 2023
hadley added a commit that referenced this issue Nov 23, 2023
hadley added a commit that referenced this issue Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants