r-lib / vctrs

Generic programming with typed R vectors
https://vctrs.r-lib.org
Other
284 stars 66 forks source link

Support for sfc lists #989

Open lionel- opened 4 years ago

lionel- commented 4 years ago

From https://github.com/r-lib/vctrs/issues/985#issuecomment-608260268.

Since we're moving away from unstructured dispatch, we need to implement special support for other mechanisms like AsIs wrappers. sfc lists might need special support, but the general mechanism that would support them isn't clear.

sfc is a bit special because combination of disparate geometry types like sfc<point> and sfc<line> falls back to an abstract geometry type sfc<ANY>, something we generally don't do in vctrs (like falling back to deparsing or chopping). Combinations with types outside the sfc hierarchy should still be an error.

A similar type to sfc is list_of(), and the abstract geometry list is like a list(). Maybe a first step towards supporting sfc would be to implement a variant of list_of() that falls back to list as soon as one element doesn't have a common type?

(Ideally combining a sfc<polygon> and a sfc<multipolygon> would return the latter. Combining with something that isn't compatible falls back to the abstract type sfc<ANY>. These more refined rules aren't implemented yet though.)

lionel- commented 4 years ago

If we can't solve this for vctrs 0.3.0, we could special-case sfc until we figure this out.

lionel- commented 4 years ago

I'm a bit wary about providing general hooks in the coercion mechanism because I think it needs to be clearly structured to keep the system transparent. But maybe the way of supporting these special classes (AsIs and sfc) would be a second layer of double dispatch called from vec_default_ptype2() and that allows inheritance. We wouldn't document this very prominently, and this would be marked experimental. It could be conceived as a way of experimenting with special type combination semantics until the general mechanism makes it way into vctrs.

Maybe with two separate generics vec_default_ptype2_left() and vec_default_ptype2_right() (the latter called from vec_default_ptype2_left.default()). A method could decline to handle a class by calling NextMethod() instead of throwing, this would allow sfc to be wrapped by AsIs. This may become a dark corner of vctrs with weird or inconsistent behaviour when multiple special classes are involved.

krlmlr commented 4 years ago

Is this similar to templates in C++? sfc_POINT is more like sfc.

I'd like to propose vec_stype(x, ...) with implementations: POSIXct -> POSIXt, POSIXlt -> POSIXt, sfc_* -> sfc, AsIs -> NextMethod() . vec_ptype2() would dispatch over vec_stype(x) and vec_stype(y) without inheritance, vec_stype() is a regular S3 method with full S3 dispatch (perhaps with inheritance prohibited?) that needs to be registered in the vctrs namespace.

I don't understand why we need _left() and _right() methods.

lionel- commented 4 years ago

@krlmlr Left and right would be needed for cast and I'd prefer ptype2 to be symmetric. It also simplifies implementations because you know exactly which argument was dispatched on. I don't think these generics should have short names because they would be experimental stopgaps. Finally POSIXlt is not really a concern.

lionel- commented 4 years ago

Interestingly, it doesn't look like the geovctrs classes would require a special mechanism. @paleolimbot There is no fallback to an abstract geometry class in your package right?

paleolimbot commented 4 years ago

I don't think so...I'm planning to keep every feature more or less independent and not rely on casting to ensure a common geometry type.

I did have a version working for a bit that used an abstract base class and used vec_ptype2.base_class.base_class() to do that kind of thing...is that what is going to stop working?

paleolimbot commented 4 years ago

Err...maybe I do. geo_wkt(), geo_wkb(), and other inherit from "geovctr". I don't use it for anything vctrs related that I know of though.

lionel- commented 4 years ago

I did have a version working for a bit that used an abstract base class and used vec_ptype2.base_class.base_class() to do that kind of thing...is that what is going to stop working?

Yes there will be no inheritance possible in the next version of vctrs.

paleolimbot commented 4 years ago

I'm almost certainly out of my depth here, but would roxygen be a way to get around some of this? If I can't use vec_ptype2.sfc.*() methods, this bit of code is going to be totally insane (the vec_cast() methods are ok because vec_ptype() of an sfc is currently a sfc_GEOMETRY).

Maybe something like this?

#' @vec_ptype2 geo_wkb sf::sfc_MULTILINESTRING geo_wkb()
#' @vec_ptype2 geo_wkb sf::sfc_LINESTRING geo_wkb()
...

...could generate the methods and dynamic registration code for both directions?

(maybe the real problem is that between me and sf we've defined too may vctrs classes?)

lionel- commented 4 years ago

I think we need to identify the general pattern that needs to be implemented in the coercion system to support this kind of classes. But that isn't a priority for the next version of vctrs which focuses on the dplyr 1.0 release.