Polygon performance #109
Polygon performance #109
Conversation
Co-authored-by: Anshul Singhvi <[email protected]>
Co-authored-by: Anshul Singhvi <[email protected]>
Co-authored-by: Anshul Singhvi <[email protected]>
| # TODO add this to `Extents.union` for any `Tuple/AbstractArray` point | ||
| function _union(extent::Extents.Extent, point::Tuple) | ||
| X = min(extent.X[1], point[1]), max(extent.X[2], point[1]) | ||
| Y = min(extent.Y[1], point[2]), max(extent.Y[2], point[2]) | ||
| return Extents.Extent(; X, Y) | ||
| end | ||
|
|
There was a problem hiding this comment.
want to move this to rafaqz/Extents.jl#24 or maybe GeoInterface?
It would be cool to have Extents.union(extent, geometry).
There was a problem hiding this comment.
Yeah I will, but not worth waiting for the release cycle for this PR
There was a problem hiding this comment.
We can add it for AbstractArray and NamedTuple as well
There was a problem hiding this comment.
Annoying we cant do it for all GeoInterface.jl points...
|
Ok its running all the test geometry checks twice. This should be good to go |
|
Good to have this 👍🏻. Might be good to comment on how you go about finding these optimizations?Similar work probably is required for GeoArrow (once there) and other packages when we want the most performance. |
|
How I found this was putting a Not very fancy... but "print summary metrics while slow things run" is a good motto I guess |
For
Shapefile.Polygonthis PR speeds up looping overGI.getgeom(poly, i)by ~100x for the first run and ~1000x after that (for very large shape files at least!)Vector{Vector{Int}}and only calculates it on the firstgetgeom.Extents.intersectsbefore actually checking_inringfor another more modest speedup.It specifically does not eagerly fill the cache on construction, because some use cases don't need it, like
GI.getringwhich returns them in the current order (which is 20x faster for e.g. calculating hulls or rasterization).Users may also only want a subset of polygons, and this skips doing most of the work in that case.
@asinghvi17 if you want to review that would be great :)
(there is a tiny bit of type instability but this will be fixed in Extents.jl its just lack of inlining stopping const prop)