Methods for sf objects

[ateucher]

Hey @sckott - I'm not sure you'll want to merge this yet, but thought I'd let you know how it was going. I think it's mostly working well, but obviously have to flesh out the tests. If you haven't seen it, the sf vignette gives a good overview of the sf and related classes.

[edzer]

This has now changed; sfc objects have a crs attribute which is a list with a epsg integer and a proj4string proj4string. I guess that only in case epsg is 4326 you can directly convert to geojson, otherwise you need to st_transform into epsg 4326.

[ateucher]

Oh, good to know, thanks @edzer. And thanks very much for taking the time to look.

Yes, I've been struggling with what to do with non-EPSG:4326 CRS's. As written, the sf to geojson methods don't depend on the sf package, adding st_transform would require it. @sckott what do you think, as I know you were hoping to keep the dependencies down? We could add it in Suggests (which I think we would need to anyways for tests), then conditionally transform to 4326 (with a message) if sf is available, otherwise throw an error telling the user they need to transform it? Or bite the bullet and add it in Imports, as I think (hope!) sf will replace sp as the standard.

[sckott]

I lean towards Suggests, but if needs to go in Imports, so be it

[ateucher]

Ok I'll go with suggests. I wonder if in the same vein we can eventually move sp and rgdal to Suggests if can parse sp to json without them

[sckott]

I wonder if in the same vein we can eventually move sp and rgdal to Suggests if can parse sp to json without them

that's my hope! at least rgdal via #41 - with geojson's ability to parse sp classes to json we don't have support for all sp classes

[edzer]

Nothing wrong with Suggests: as long as you use constructs like this. Also pls use sf::st_crs rather than accessing attributes directly; works for more cases (sf and sfc), and makes your code live longer.

[ateucher]

Yep, that is how I was planning on doing it. I think I need to access the crs attribute directly though, because we want to first check the CRS only to see if we need st_transform, otherwise we can go directly to json without depending on sf. I.e.:

epsg <- attr(x, "crs")[["epsg"]]
if (epsg != 4326)) {
  if (!requireNamespace("sf", quietly = TRUE)) {
    stop("Your input is not in a CRS that geojson supports and you don't have the 'sf' package installed. Please install and try again")
  } else {
    x <- sf::st_transform(x, 4326)
  }
}
## turn x into json

Unless I'm missing something else...?

[edzer]

No that should work - I didn't anticipate you want to write the geojson without sf::st_write, good luck!

[ateucher]

lol, thanks! Unless I'm really deceiving myself, because sf and geojson both use simple features, it's relatively simple to parse an sf[cg] object into a list that mirrors the geojson structure, then use JSONlite to turn it into a geojson character string. I haven't dealt with Z or M values yet, and I'm sure there are other cases that I'm missing, but inital tests are promising.

[edzer]

Lucky you - the geojson standard excludes Z and M, and constraints to the first 7 types.

[ateucher]

A couple of notes on the draft new GEOSJON standard:

1. With regards to CRS, there is this:

   However, where all involved parties have a prior arrangement, alternative coordinate reference systems can be used without risk of data being misinterpreted.

   So there may be cases where converting to WGS84 is not desirable by the user. I think there are two options:

   1. Add an argument (which would have to be added to all methods) convert_wgs84with a default of TRUE, to allow user to override. OR
   2. Proceed with the conversion regardless of CRS and throw a warning if it's not WGS84.

2. With regards to Z (elevation):

   Altitude or elevation MAY be included as an optional third element.

   So we CAN keep Z in coordinates, but not M.

[ateucher]

@sckott any thoughts on number 1 above?

[sckott]

@ateucher Seems like since WGS84 is the standard for geojson we could just convert it to that, but letting the user toggle that will make more people happy perhaps?

[ateucher]

Thanks @sckott that works for me. Selfishly, I want to be able to bypass transformation to use in rmapshaper

[sckott]

@ateucher wait, which one works?

[ateucher]

@sckott I think this is ready for merge (or further review/testing by other eyes). It's passing on my fork, failing here with the map_gist tests.

I omitted dealing with non-epsg:4326 CRS until we decide to do it package-wide - didn't seem right to implement it only in these methods...

[ateucher]

Just playing with Travis...

[sckott]

i think its failing cause GITHUB_PAT is not avail on PR's, so if its just those tests failing all is good

[sckott]

that makes sense to implement converting pkg wide later

[sckott]

Thanks for the tests!

Can you add some examples though for the different sf classes for both files

[ateucher]

Oh right, examples. Yep, will do.

Re Travis failing, I figured that it was GITHIB_PAT.

[ateucher]

@sckott I think this is good to go now. I added examples (hopefully they're enough), and methods for geojson_write.

[sckott]

Sweet, will look in morning

[sckott]

LGTM

nice work sir @ateucher

[ateucher]

Woot! Thanks :)

[github-actions]

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.