-
Notifications
You must be signed in to change notification settings - Fork 37
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
S7 arrays #401
Comments
FWIW - Looks like it's currently in draft stage (#434) so hopefully going to be added. |
yay!
…On Fri, 6 Sept 2024 at 12:24, Tim Taylor ***@***.***> wrote:
FWIW - Looks like it's currently in draft stage (#434
<#434>) so hopefully going to be
added.
—
Reply to this email directly, view it on GitHub
<#401 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHQGSVONYUHLOCFFZ2FQK6TZVGGGZAVCNFSM6AAAAABEYH7X42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZTHA2DKMBYGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This is implemented in the dev version of S7. new_class("my_array", parent = class_array) |
@t-kalinowski how does that work? what type of array do you get? |
length 0 logical (this makes the most sense, for the same reason that library(S7)
new_class("my_array", parent = class_array)
#> <my_array> class
#> @ parent : S3<array>
#> @ constructor: function(.data, dim, dimnames) {...}
#> @ validator : <NULL>
#> @ properties :
new_class("my_array", parent = class_array)()
#> <my_array> logi[0 (1d)] Created on 2024-10-11 with reprex v2.1.1 |
@t-kalinowski hmmm, that feels weird to me because |
I agree, this should be easy. We only provide I wonder if we should introduce a class_array & class_logical |
It feels like we should discuss this design a bit more before committing to it in a release. Maybe roll back for this release? |
This sounds nice to me ((without further deep thinking though) independent of |
One could think of matrix and array as containers. Specifying the contents of a container is typically done through parameterization. Parameterized types would be interesting. Maybe they are represented by a higher order function that accepts the type parameters and returns a constructor that ensures the right mode? The interface might look like: Matrix <- new_parameterized_class("Matrix", class_matrix, list(T = class_vector))
Matrix(class_numeric)(1:10, 2) Or, we more directly address the contract of the dim attribute presence using something like Haskell type classes: Matrix <- new_predicate("Matrix", function(self) !is.null(attr(self, "dim"))) But that will really slow down dispatch. The methods package did explore this approach with |
Quoting from
So, it seems we need to think of this in terms of "implicit" classes. We currently handle the implicit "numeric" class using a union of Union classes cannot be used as a parent in > new_class("foo", class_numeric)
Error: `parent` must be an S7 class, S3 class, or base type, not an S7 union. Some initial thoughts:
One temptation is to open the barn doors and think through what it means to support arbitrary "implicit" classes, similar to a mixin class in Python or a Rust trait—where a generic can make assumptions about the object’s properties. For example, in #433, we seem to be reaching for a class_scalar <- new_class(
"class_scalar",
validator = function(self)
if (!identical(length(self), 1L))
"must be length 1"
) Or, continuing along this desire path, one could further imagine implicit types as peers of regular types, allowed to be children of union type and composable with base types. class_scalar <- new_class(
"class_scalar",
parent = class_vector,
validator = function(self)
if (!identical(length(self), 1L))
"must be length 1"
)
class_string <-
new_class("class_string", parent = c(class_character, class_scalar)) This would be straightforward to implement from the constructor's side. However, the challenge is method dispatch with base objects that lack a I see no efficient way to support:
Another question to consider: What happens when a Foo <- new_class(
"Foo",
parent = class_double,
properties = list(dim = class_integer)
)
foo <- Foo(as.double(1:4), c(2, 2))
.class2(foo) # "Foo" "double" "S7_object"
.class2(unclass(foo)) # "matrix" "array" "double" "numeric" There is already an inconsistency with the implicit "numeric" class, where we intentionally omit dispatching on Foo <- new_class("Foo", class_double)
foo <- Foo()
.class2(foo) # "Foo" "double" "S7_object"
.class2(unclass(foo)) # "double" "numeric" Perhaps, we continue with the existing precedent and intentionally limit support for the concept of implicit classes in S7, beyond some minimal support in E.g., we offer no default constructor for > new_class("Foo", class_POSIXt)()
Error:
! S3 class <POSIXt> doesn't have a constructor
> new_class("Foo", class_numeric)()
Error:
! `parent` must be an S7 class, S3 class, or base type, not an S7 union. This could be via Another idea is introducing a counterpart to |
It seems like we should at least merge the existing implicit classes from In the general case, I think we'll start running into issues similar to those of multiple inheritance. How do we linearize the hierarchy with these implicit/dynamic classes? One way to make implicit classes semi-efficient would be to figure out the classes upon class definition and upon each modification of the object, to the extent possible, alongside validation. Definitely worth a discussion at our next meeting. |
It's worth noting that it's not valid to have a library(S7)
Foo <- new_class("Foo", properties = list(
dim = new_property(NULL | class_integer)
))
foo <- Foo()
foo@dim <- 1:3
#> Error in `@<-.S7_object`(`*tmp*`, "dim", value = 1:3): invalid first argument, must be vector (list or atomic)
dim(foo) <- 1:3
#> Error in dim(foo) <- 1:3: invalid first argument, must be vector (list or atomic) This error message comes from base R |
That could be loosened to allow |
Is there a good way to create an S7 object which inherits from "array" (or "matrix", but my specific use case is "array")? If not, will this be added?
The text was updated successfully, but these errors were encountered: