-
Notifications
You must be signed in to change notification settings - Fork 10
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
Use GeometryBasics types #36
Comments
I think it makes sense, yes. One alternative that is worth discussing is an approach like https://github.com/visr/GeoJSONTables.jl/tree/rfc. That works with the new GeoInterface (as it was end of 2019). It's quite simple and fast, because we just read the whole file as a JSON3 object, and then wrap the coordinates in structs like: struct Polygon{T, S, TT} <: Geometry{T}
json::JSON3.Array{T, S, TT}
end There is also visr/GeoJSONTables.jl#3 which switches to GeometryBasics, and puts the metadata in there also. Though it's worth keeping in mind that JuliaGeometry/GeometryBasics.jl#173 removes a lot of the metadata functionality from GeometryBasics. And I believe, similar to #23, this slows parsing quite a bit, because you do more work (but then get a more usable geometry type back). There are some simple benchmarks in #23. In any case I think adding a Tables interface for FeatureCollections, like GeoJSONTables, Shapefile and ArchGDAL, is a good idea. |
Ok interesting, I do like that lazy Table approach. And as with Shapefile.jl, I would prefer not to lose the bbox metadata given the changes to GeometryBasics.jl. |
I also really like the GeoTables.jl idea. Were you thinking we could just wrap Tables.jl and define some blessed |
I assume you're referring to this GeoJSONTables README text:
The name GeoTables is now used by https://github.com/JuliaEarth/GeoTables.jl, which is different from what I was talking about. So to distinguish perhaps we should call it GeoInterfaceTables.jl. But indeed it would be something like what you mention. I wasn't sure if the geometries should be accesible from a property like |
Ok, I'm keen to switch GeoJSON over to GeoJSONTables style instead of GeometryBasics. Just to clarify what needs to be done, my understanding of the structure would be:
Basically this error needs to be replaced with the code to parse arbitrary GeoJSON
Edit: don't worry, its done visr/GeoJSONTables.jl#11 Do you think we should do that and completely replace the code here with updated GeoJSONTables.jl code? |
Yes those are indeed the things that need to be done, thanks for working on them already! If we indeed make sure that the GeoJSONTables approach becomes a fully featured GeoJSON package, I think it'd be better to replace the code here with the updated GeoJSONTables code. We could do something like https://github.com/JuliaGraphs/Graphs.jl#project-status to make sure the commits from both packages are retained in the history. I just gave you push access to GeoJSONTables.jl. I'll merge the old rfc branch into master, and review your PR to update it. Shall we hash things out there to make sure it's ready for when the new GeoInterface lands? |
Fixed by #42. |
When GeometryBasics works with GeoInterface, we will need to switch to using GeometryBasics.jl types here, as the GeoInterface types used currently will go away.
Does this make sense? Just adding this before I write it in case there are other opinions.
The text was updated successfully, but these errors were encountered: