afrimapr / afrihealthsites

access to geographic locations of african health sites from different sources
https://wellcomeopenresearch.org/articles/5-157
GNU General Public License v3.0
31 stars 4 forks source link

Error in st_geometry.sf(x) : attr(obj, "sf_column") does not point to a geometry column. #1

Closed andysouth closed 4 years ago

andysouth commented 4 years ago

Intermittently this error appears

library(afrihealthsites)
afrihealthsites("mali")

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"?

But when I browse to debug the error doesn't happen.

Can anyone fix please ?

dickoa commented 4 years ago

It's difficult to reproduce, is it random ?

andysouth commented 4 years ago

Thanks, for me it seems to occur just first time function is used after package has been installed (and in any examples in R CMD check). At least it doesn't happen all the time ;-)

I think it may have something to do with an absence of sf see this https://github.com/r-spatial/sf/issues/1185#issuecomment-551133412.

But sf is imported to the package. Hmmm

dickoa commented 4 years ago

It's really hard to track but I will keep an eye on it and as soon as I can reproduce or just experience this error I will see if I can fix it.

andysouth commented 4 years ago

@Robinlovelace have you come across this before ?

Robinlovelace commented 4 years ago

Thanks for flagging this @andysouth, it's an unusual error message. Can you provide a reproducible example?

andysouth commented 4 years ago

@robinlovelace I've struggled to make it reproducible, when I get close to it, it disappears.

I added the function test_error() to the package. When I load the package, the first time I run test_error() it does give the error. The 2nd time I run it, it doesn't.

If I remove the row subset line I get no error, and when I replace the sf object with the example one from the sf package I also get no error.

The error does occur on shinyapps.io so not just my setup.

Be good to know if the error occurs for you. Thanks.

test_error <- function( plot = 'sf' ) {

  sf1 <- sf_healthsites_af

  filter_country <- tolower(sf_healthsites_af$iso3c) %in% "nga"

  ###LINE BELOW CAUSES THE ERROR AT EXAMPLES CHECK - without it the check passes
  sf1 <- sf1[filter_country,]

  if (plot == 'mapview') print(mapview::mapview(sf1))
  else if (plot == 'sf')  plot(sf::st_geometry(sf1))
}
Robinlovelace commented 4 years ago

Thanks @andysouth but can you start the 'reprex' with commands that install and load the necessary packages to reproduce the error? I'm behind the curve on the various dependencies you're using here and get this:


test_error <- function( plot = 'sf' ) {

  sf1 <- sf_healthsites_af

  filter_country <- tolower(sf_healthsites_af$iso3c) %in% "nga"

  ###LINE BELOW CAUSES THE ERROR AT EXAMPLES CHECK - without it the check passes
  sf1 <- sf1[filter_country,]

  if (plot == 'mapview') print(mapview::mapview(sf1))
  else if (plot == 'sf')  plot(sf::st_geometry(sf1))
}
test_error()
#> Error in test_error(): object 'sf_healthsites_af' not found

Created on 2020-03-23 by the reprex package (v0.3.0)

andysouth commented 4 years ago

Apologies @Robinlovelace I could have been clearer. I suggest you install the afrihealthsites package :

remotes::install_github("afrimapr/afrihealthsites")
library(afrihealthsites)
test_error()
dickoa commented 4 years ago

I think I have a better understanding of the error. The first time you run the code, sf is not loaded on memory but the second you have sf. The main issue is subsetting the data without sf loaded turn your geometry object into a list.

library(afrihealthsites)
sf1 <- sf_healthsites_af
## > class(sf1$geometry)
## [1] "sfc_POINT" "sfc"      
## > class(sf1[1:2, ]$geometry)
## [1] "list"

When sf is loaded you can subset the data while keeping the class of the geometry object. When you run the code below in a fresh session it works.

library(afrihealthsites)
library(sf)
sf1 <- sf_healthsites_af
class(sf1$geometry)
## [1] "sfc_POINT" "sfc"      
class(sf1[1:2, ]$geometry)
## [1] "sfc_POINT" "sfc"      

I think you have two options, you can use this [sf::"[.sf"](https://github.com/r-spatial/sf/blob/master/R/sf.R#L316) directly in your code or just load sf while you load your package may there's another option that I'm not aware of.

Thanks for the reproducible example, it was not easy to spot this one and I hope it helps

Ahmadou

andysouth commented 4 years ago

Excellent, thanks @dickoa that makes sense, and explains why every time I got close it vanished. Excuse my ignorance, how can I rewrite the row subsetting (the line below) within the function so that it uses the sf method ? sf1 <- sf1[filter_country,]

Robinlovelace commented 4 years ago

remotes::install_github("afrimapr/afrihealthsites")
#> Using github PAT from envvar GITHUB_PAT
#> Skipping install of 'afrihealthsites' from a github remote, the SHA1 (daefef09) has not changed since last install.
#>   Use `force = TRUE` to force installation
library(afrihealthsites)
test_error <- function( plot = 'sf' ) {

  sf1 <- sf_healthsites_af

  filter_country <- tolower(sf_healthsites_af$iso3c) %in% "nga"

  ###LINE BELOW CAUSES THE ERROR AT EXAMPLES CHECK - without it the check passes
  sf1 <- sf1[filter_country,]

  if (plot == 'mapview') print(mapview::mapview(sf1))
  else if (plot == 'sf')  plot(sf::st_geometry(sf1))
}
test_error()
#> Error in st_geometry.sf(sf1): attr(obj, "sf_column") does not point to a geometry column.
#> Did you rename it, without setting st_geometry(obj) <- "newname"?
library(sf)
#> Linking to GEOS 3.8.0, GDAL 3.0.4, PROJ 7.0.0
test_error()

Created on 2020-03-23 by the reprex package (v0.3.0)

Robinlovelace commented 4 years ago

Could add sf as a Depends or load it manually before it gets to it. Agree it's strange, never seen it before.

andysouth commented 4 years ago

Thanks both I'll probably add sf Depends as the easy solution. The package does rely on it.

Robinlovelace commented 4 years ago

Another example showing the issue is not specific to your package:

nz = spData::nz
nz[1, ]
#>        Name Island Land_area Population Median_income Sex_ratio
#> 1 Northland  North  12500.56     175500         23400 0.9424532
#>                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     geom
#> 1 1745493, 1740539, 1733165, 1720197, 1709110, 1701512, 1694072, 1698473, 1703769, 1706031, 1701820, 1693314, 1688395, 1677966, 1662101, 1633100, 1632333, 1604938, 1611888, 1615313, 1615273, 1607402, 1586363, 1568217, 1572430, 1580992, 1587965, 1601327, 1598604, 1602303, 1610993, 1618217, 1625654, 1619611, 1626313, 1631049, 1626225, 1639245, 1635582, 1639613, 1648872, 1651391, 1658794, 1664018, 1667504, 1676774, 1684933, 1687775, 1698281, 1698221, 1702334, 1717303, 1722718, 1721686, 1733193, 1733658, 1739227, 1736022, 1740876, 1740417, 1734255, 1728575, 1726888, 1735540, 1731746, 1736045, 1743202, 1745493, 6001802, 5995066, 5989714, 5980078, 5986672, 5996205, 5996670, 5988332, 5984809, 5975136, 5971967, 5971971, 5989681, 6005589, 6027765, 6066188, 6068917, 6105748, 6107106, 6111815, 6122911, 6138887, 6168009, 6184787, 6190525, 6187485, 6191874, 6190815, 6179319, 6165379, 6147279, 6141248, 6138839, 6131232, 6125452, 6131536, 6142057, 6146263, 6132045, 6128431, 6126997, 6135510, 6130516, 6132003, 6122581, 6127265, 6121315, 6113351, 6113546, 6096136, 6093768, 6095681, 6089921, 6084337, 6071541, 6064257, 6060695, 6054848, 6045543, 6030364, 6038727, 6038983, 6033883, 6032640, 6024546, 6011618, 6009537, 6001802
plot(nz[1, ])
#> Warning in data.matrix(x): NAs introduced by coercion

#> Warning in data.matrix(x): NAs introduced by coercion
#> Warning in min(x): no non-missing arguments to min; returning Inf
#> Warning in max(x): no non-missing arguments to max; returning -Inf
#> Warning in min(x): no non-missing arguments to min; returning Inf
#> Warning in max(x): no non-missing arguments to max; returning -Inf
#> Error in plot.window(...): need finite 'xlim' values

library(sf)
#> Linking to GEOS 3.8.0, GDAL 3.0.4, PROJ 7.0.0
nz[1, ]
#> Simple feature collection with 1 feature and 6 fields
#> geometry type:  MULTIPOLYGON
#> dimension:      XY
#> bbox:           xmin: 1568217 ymin: 5971967 xmax: 1745493 ymax: 6191874
#> projected CRS:  NZGD2000 / New Zealand Transverse Mercator 2000
#>        Name Island Land_area Population Median_income Sex_ratio
#> 1 Northland  North  12500.56     175500         23400 0.9424532
#>                             geom
#> 1 MULTIPOLYGON (((1745493 600...
plot(nz[1, ])

Created on 2020-03-23 by the reprex package (v0.3.0)

I suspect there may be an sf buried in there but am not sure what it may be.

Thanks both I'll probably add sf Depends as the easy solution. The package does rely on it.

That's quite a drastic solution. Another option is to add this in the relevant place:

if (requireNamespace("sf", quietly = TRUE)) {
   # stuff that requires sf
} 
andysouth commented 4 years ago

Thanks @Robinlovelace, given that this package is always going to return sf objects, depending on sf seems OK to me and may protect users from getting bitten by this error ? I've already added the Depends for now. Good to know that have another option if needed later.

Robinlovelace commented 4 years ago

depending on sf seems OK to me and may protect users from getting bitten by this error ?

OK for sure, stplanr used to depend on sp and it worked fine so :+1: from me.

dickoa commented 4 years ago

Looks like it's the best option since the use package:::fun i.e non exported functions is not standard and CRAN doesn't like this idiom either. Thus using sf:::"[.sf" can be problematic long term therefore loading the package is probably safer. More on this thread https://github.com/r-spatial/sf/issues/970

Robinlovelace commented 4 years ago

therefore loading the package is probably safer. More on this thread r-spatial/sf#970

:+1: and great link to conversations going on upstream.

andysouth commented 4 years ago

Thanks both for your help sorting this, closing now.