Skip to content
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

Update for the new GeoInterface #72

Merged
merged 16 commits into from
May 23, 2022
Merged

Update for the new GeoInterface #72

merged 16 commits into from
May 23, 2022

Conversation

rafaqz
Copy link
Member

@rafaqz rafaqz commented Apr 27, 2022

See JuliaGeo/GeoInterface.jl#33

There are some issues with how to fit the odd format of Shapefiles - mixed multipoligons with rings and holes - into the OGC standard and GeoInterface. We need to work out how to efficiently index holes with gethole when we have to manually check.

@rafaqz rafaqz changed the title update for the new GeoInterface WIP update for the new GeoInterface Apr 27, 2022
@rafaqz rafaqz marked this pull request as draft April 27, 2022 14:14
@visr
Copy link
Member

visr commented Apr 27, 2022

Nice work! How do you see this in relation to #39?

@rafaqz
Copy link
Member Author

rafaqz commented Apr 27, 2022

Yeah. If we do that, we won't need this PR, because we will have the interface in GB anyway. So this is kind of temporary really.

But there are three unmerged GB PRs here now and I dont know where they are at or which to move forward.

Im also not totally sure if the upfront conversion to GB types is the best approach. With shapefiles we know the bounding boxes of all objects and can use that information to reduce work. But with GeometryBasics conversion all shapes will be sorted into rings/holes on load. That can be a huge and unnessary overhead if we only need to analyse a small region of a large shape file. On the other hand I can also see how annoying shapefiles are to fit into the GeoInterface.

@rafaqz
Copy link
Member Author

rafaqz commented Apr 28, 2022

Seems like this will kill the other GeometryBasics PRs here? JuliaGeometry/GeometryBasics.jl#173

We need metadata for the bounding box, which would be a shame to lose.

@visr
Copy link
Member

visr commented Apr 28, 2022

Yeah they would need to be adapted at least. Indeed these sort of issues show the difficulty of putting everything into shared geometry types. Things like metadata and extents would understandably become out of scope for GeometryBasics.

In the end what matters the most is easy interop. If we can show that we can use Shapefile + GeoInterface to easily get either GEOS geometries or GeometryBasics ones, that would be great. The Shapefile geometry structs are not meant for direct use for most cases. So even a struct that wraps a stream or byte array would be enought (like GeoJSONTables, see JuliaGeo/GeoJSON.jl#36 (comment)).

@rafaqz
Copy link
Member Author

rafaqz commented May 9, 2022

This is working, just waiting on the new GeoInterfaceRecipes subpackage to be merged and registered for plot recipes here.

  • files are broken into object types with relevent methods contained
  • because polygons here are really multipoligons, there are some lazy objects that handle getting the polygons and rings from them.
  • everything 3d is actually 3d now.
  • we have getring in GeoInterface to skip the whole searching for exteriors and holes step, which is much faster for rasterizing and plotting.

@visr my long term plan is to replace this package with the 100% lazy version we spoke about. And call it Shapefiles.jl so we can just use Shapefile(filename) as an object. And make that object automatically act as a feature table.

But for now this is working, and should actually be much faster to load, plot and rasterize than before.

@visr
Copy link
Member

visr commented May 9, 2022

Cool! Besides the new GeoInterface, what (if any) is breaking about this? Does e.g. the README quick start example still work without changes?

@rafaqz
Copy link
Member Author

rafaqz commented May 9, 2022

There shouldn't be any user facing changes, except that e.g. coordinates in the readme would be 3d (length 3 coord vectors) now for any Z shape. Although... that will actually break because of the exterior/holes sorting algorithm.

I'm not sure what to do about that actually.

@rafaqz
Copy link
Member Author

rafaqz commented May 19, 2022

@evetion and @visr, do you have any ideas on what to do about 3d polygons and the geointerface. I guess we need a 3d inpolygon algorithm to separate exterior and interior rings?

We just pretended they were 2d before.

@evetion
Copy link
Member

evetion commented May 19, 2022

You can still pretend that they're 2d, as they're not truly 3d, but more like 2.5d. So the inpolygon just works with the first two coordinates.

@rafaqz
Copy link
Member Author

rafaqz commented May 19, 2022

Ah ok thats what I've done anyway. So its the same as before, but the actual coordinates returned are 3d.

@visr
Copy link
Member

visr commented May 19, 2022

Feel free to drop julia pre-1.6 here by the way.

README.md Outdated Show resolved Hide resolved
@rafaqz rafaqz force-pushed the rs/geointerface branch from 1fb47da to 806a376 Compare May 19, 2022 12:47
@rafaqz rafaqz changed the title WIP update for the new GeoInterface Update for the new GeoInterface May 19, 2022
@rafaqz rafaqz marked this pull request as ready for review May 19, 2022 13:16
@rafaqz rafaqz requested a review from evetion May 19, 2022 13:16
@rafaqz
Copy link
Member Author

rafaqz commented May 19, 2022

If added convert methods, which should also allow writing to shapefile from other formats with a reboot of #34

src/multipatch.jl Outdated Show resolved Hide resolved
src/multipatch.jl Outdated Show resolved Hide resolved
src/table.jl Show resolved Hide resolved
@evetion
Copy link
Member

evetion commented May 21, 2022

Because of the major restructuring, I can't really tell what's new code and old. I have a few small comments, otherwise LGTM.

@rafaqz rafaqz force-pushed the rs/geointerface branch from 952ce06 to 2fff2ab Compare May 23, 2022 13:57
@rafaqz
Copy link
Member Author

rafaqz commented May 23, 2022

Yes apologies about the restructure, it got too tedious scrolling around and finding things with the old structure. A lot of the old code is changed and simplified as well.

The main new types are LinearRing, LineString and SubPolygon that are lazy objects over the internals of other objects so that GeoInterface.jl methods make sense (SubPolygon is so named because Polygon here has MultiPolygonTrait in geointerface, and SubPolygon has the actual PolygonTrait)

Also: this should be good to merge

@rafaqz rafaqz merged commit e360fe0 into main May 23, 2022
@rafaqz rafaqz deleted the rs/geointerface branch May 23, 2022 17:46
@rafaqz
Copy link
Member Author

rafaqz commented May 23, 2022

Merged so we can get ArchGDAL tests passing ASAP. There may be some changes to add to this afterwards, but it should mostly be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants