GIScience / openrouteservice-r

:globe_with_meridians: query openrouteservice API from R
https://giscience.github.io/openrouteservice-r/
Apache License 2.0
98 stars 23 forks source link

Using geojsonsf::geojson_sf to parse isochrone result seems to be wrong #65

Closed etiennekintzler closed 4 years ago

etiennekintzler commented 4 years ago

I am using the version 0.4.0 of openrouteservice-r package and version of geojsonsf 1.3.3 and use ors_isochrones function as follow :

res_sf <- ors_isochrones(
  locations=data.frame(lon=1.703294, lat=48.98969),
  range=1800, 
  interval=300, 
  output="sf",
  profile="cycling-electric"
)

Then resulting output is broken for a lot of functions from sf package. The recurring error is Not compatible with requested type: [type=character; target=integer]..

For instance using sf::st_intersection(res_sf, res_sf)

Error in CPL_geos_op2(op, st_geometry(x), st_geometry(y)) : 
  Not compatible with requested type: [type=character; target=integer].

Using : sf::as_Spatial(res_sf)

Error in CPL_geos_is_empty(st_geometry(x)) : 
  Not compatible with requested type: [type=character; target=integer].

Using sf::st_transform(res_sf, crs=3857) :

Error in CPL_transform(x, crs$proj4string) : 
  Not compatible with requested type: [type=character; target=integer].

etc.

The solution is to just use sf::st_read or (sf::read_sf which returns a tibble) on the raw text (the output of ors_isochrones with output=text). I think it should be the default way to convert the GeoJSON text output to sf object, instead of using geojsonsf. I can do the PR if necessary.

etiennekintzler commented 4 years ago

A quick fix (without resorting to sf::read_sf) is to pass argument crs=4326 as in geojsonsf::geojson_sf(geojson_to_parse, crs=4326).

I posted the underlying issue in geojsonsf package (https://github.com/SymbolixAU/geojsonsf/issues/83).

aoles commented 4 years ago

Thanks a lot @etiennekintzler for taking the time to investigate the problem and report it, I really appreciate it.

If I understood it correctly, this is a transient issue which will resolve itself with the next update of geojsonsf, right? I would rather stick to the current solution than switch to sf::st_read because of performance reasons.

What I don't get is why specifying crs = 4326 to geojsonsf::geojson_sf addresses the problem. Doesn't this setting correspond to WGS 84 which, according to the documentation, is the default anyway?

geojson_sfc and geojson_sf automatically set the CRS to WGS 84. The fields crs and proj4string let you to overwrite the defaults.

Cheers, Andrzej

dcooley commented 4 years ago

Hi Andrzej,

What I don't get is why specifying crs = 4326 to geojsonsf::geojson_sf addresses the problem. Doesn't this setting correspond to WGS 84 which, according to the documentation, is the default anyway?

To add some detail (I'm the geojsonsf author), this isn't an issue with the CRS value itself, but rather the type of the value. In sf >= v0.9 the crs object is now a list of two strings, input and wkt reference, rather than an int (epsg) and a string (proj4string) as it was in < v0.9

So I think the issues here are conflicts between the expecations of sf < v0.9 and >= v0.9.

If I understood it correctly, this is a transient issue which will resolve itself with the next update of geojsonsf, right? I

yes that is the intention.

aoles commented 4 years ago

Thanks for the insights @dcooley!

So what's the timeline for releasing a patched geojsonsf on CRAN? I'm wondering whether I should implement a temporary workaround rather than just wait for the upstream fix.

Cheers!

dcooley commented 4 years ago

imminently! I want to check a couple of things tomorrow but I should have it submitted before the end of the day (Australia time).

aoles commented 4 years ago

Awesome news, thanks a lot Dave!

dcooley commented 4 years ago

v2.0.0 is now on CRAN 👍

aoles commented 4 years ago

Great news, thanks a lot @dcooley !

etiennekintzler commented 4 years ago

geojsonsf is still not compatible with sf < 0.9 and now with the update I get the error Error: C stack usage 7972980 is too close to the limit.

The fix @dcooley provide is just a warning when loading the geojsonsf library. Also this warning won't appear when loading openrouteservice. Thus the user openrouteservice might still be confronted to this issue.