-
Notifications
You must be signed in to change notification settings - Fork 37
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
Allow outSR parameter to be changed in getESRIFeatureByIds #25
Comments
Essentially what I am seeing here is that getEsriFeaturesByIds() gets called when pulling the json data from the feature/map server. It is currently hardcoded for the EPSG:4326 (line 65 above). However the Lines 34 to 65 in cbcaf3b
This is an issue because we are forcing the data to come in as EPSG:4326 but then we allow the data to be defined as any CRS given to the Lines 119 to 134 in cbcaf3b
I can see this leading to some unexpected errors when trying to define a CRS. I believe sf::st_sf() only directly sets a CRS for a layer and does not attempt a transformation. I also see another (more opinionated) issue with using EPSG:4326 as the default because it means the user of the package needs to be aware that they may be introducing datum transformation shift error into their dataset if the Feature/Map Service has the data stored in a different datum than WGS1984. While this error is often relatively low (the example I often encounter is WGS1984 to NAD1983 with an error of about 2.5 feet) it is unacceptable in some cases. I see two paths to fix this.
I am going to work on coding up a pull request for option 2 for a functional example and we can discuss whether or not it should be accepted and/or what the default should be. I guess we could always make the default be |
So I've been working on a pull request for this issue and I am hitting a small snag. For the Without a way to link to GDAL, we will be limited in our inputs for the https://github.com/jacpete/esri2sf/blob/62c6e8a3368d1fe9bfe24e400110d5e8606e3fe8/R/zzz.R#L149-L172 To extend the function to be able to accept any crs format you can generally use in R (e.g., proj4string, WKT, numeric EPSG) there will need to be an interface directly to the GDAL https://github.com/r-spatial/sf/blob/405ca1ca4ebdbd5672948aea88251e5d12004740/src/gdal.cpp#L115-L128 |
Thank you for the detailed comments and the PR. I hope the sf package will address this issue as well. |
As of now, the outSR parameter in the
query
list creation is hard coded to be WGS1984 EPSG:4326. I am guessing this is legacy before the crs argument was added esri2sf. I believe this needs updated so that the output CRS can be specified correctly.esri2sf/R/zzz.R
Lines 62 to 73 in cbcaf3b
I am going to fork the repository and do some testing and may upload a pull request.
The text was updated successfully, but these errors were encountered: