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

Fix sf combination with multiple chunks #1136

Closed
lionel- opened this issue Jun 4, 2020 · 3 comments · Fixed by #1137
Closed

Fix sf combination with multiple chunks #1136

lionel- opened this issue Jun 4, 2020 · 3 comments · Fixed by #1137
Labels
bug an unexpected problem or unintended behavior op:bind-combine type:dataframe type:fallback
Milestone

Comments

@lionel-
Copy link
Member

lionel- commented Jun 4, 2020

From r-spatial/sf#1390 (comment)

sfc <- sf::st_sfc(sf::st_point())
sf <- sf::st_sf(geo = sfc)

vctrs::vec_rbind(sf, sf)
#> Simple feature collection with 2 features and 0 fields (with 2 geometries empty)
#> geometry type:  POINT
#> dimension:      XY
#> bbox:           xmin: NA ymin: NA xmax: NA ymax: NA
#> CRS:            NA
#>           geo
#> 1 POINT EMPTY
#> 2 POINT EMPTY

vctrs::vec_rbind(sf, sf, sf)
#> Error in st_geometry.sf(x) :
#>   attr(obj, "sf_column") does not point to a geometry column.
#> Did you rename it, without setting st_geometry(obj) <- "newname"?

This is a bad interaction between the c() fallback in vec_rbind(), which currently creates a sentinel column in the common type, and the same-type fallback for foreign data frames, which calls [<- to update the inputs with the common-type column structure.

@lionel- lionel- added bug an unexpected problem or unintended behavior op:bind-combine type:dataframe type:fallback labels Jun 4, 2020
@lionel- lionel- added this to the 0.3.1 milestone Jun 4, 2020
@lionel-
Copy link
Member Author

lionel- commented Jun 4, 2020

One way to fix this is to implement ptype2 methods for sf. From https://github.com/r-spatial/sf/blob/2ca6483f08555185bd55a05d478025f5d1d73b1c/R/bind.R#L18-L34

  1. Check crs is equal.

  2. Deal with geometry type.

    • For the ptype2 prototype inputs, it's always an abstract geometry.
    • For cast, do we differentiate between abstract geometries and concrete geometries? Can always cast to abstract (recompute the concrete type). For concrete geometries, must be compatible?
  3. Recompute bounding box. Not relevant for ptype2.

@lionel-
Copy link
Member Author

lionel- commented Jun 4, 2020

Another way is to remove the use of a common-type sentinel for the c() fallback in vec_rbind(). This sentinel is necessary to leverage ptype2. We could instead work at the ptype-common level (using ideas from #1128), but then I think we'd need to implement df-ptype-common. This is likely non-trivial work and we would prefer not spending too much time on fallbacks.

@lionel-
Copy link
Member Author

lionel- commented Jun 4, 2020

Of course we have the same problem when implementing vec_ptype2.sf.sf... The common type contains the sentinels and we can't properly restore the sf data frame:

sfc1 <- sf::st_sfc(sf::st_point())
sfc2 <- sf::st_sfc(sf::st_linestring())
sf1 <- sf::st_sf(geo1 = sfc1)
sf2 <- sf::st_sf(geo2 = sfc2)

vec_rbind(sf1, sf2)

=>

# Within `vec_ptype2.sf.sf()`:
data = df_ptype2(x, y, ...)
lapply(data, class)
#> $geo1
#> [1] "vctrs:::common_class_fallback"
#>
#> $geo2
#> [1] "vctrs:::common_class_fallback"

We can work around this by patching the internal parameters in .... However I'm not comfortable doing this in code that lives in a downstream package. So that would mean conditional initialisation of these methods in the vctrs package.

lionel- added a commit to lionel-/sf that referenced this issue Jun 4, 2020
lionel- added a commit to lionel-/vctrs that referenced this issue Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior op:bind-combine type:dataframe type:fallback
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant