-
Notifications
You must be signed in to change notification settings - Fork 20
Implement GeoInterface on Extent #211
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
base: main
Are you sure you want to change the base?
Conversation
|
I think this probably needs a bit more consideration because right now, a lot of methods in the ecosystem, specifically GeometryOps and Rasters, assume that the trait of an extent is nothing and dispatch based on that. So any change here would have to be accompanied by a corresponding change there. It's not technically a breaking change, but I would argue that it's breaking enough that a lot of packages will need to retro-cap their GeoInterface compat on the registry.. |
|
How many packages are we talking about, apart from GO and Rasters? One reason for this is that GeoArrow specificies a Box type, which results in a GeoDataFrame with a geometry column with Extents (our native box type), that doesn't implement GeoInterface (and thus breaks in usage). The other path is to introduce a new Box type, but that seems duplicate work, Extent clearly is our (bounding) box type in Julia. |
|
Yeah at this point it's probably just GO, Rasters, maybe ArchGDAL? We should be able to push that through but it would take some coordination. Would be a good topic to discuss on the community call and then we implement? We'd probably then also want IO support - at least to reinterpret an extent in a geometry column as a polygon or something. And we should probably have a higher level BoundingBoxTrait (or FWIW, I am broadly happy with the changes made here. But extending over the ecosystem is the thorny issue - since older versions of Rasters and GO will not work with this newer GeoInterface. We should probably just retro-cap those packages on the registry. |
|
Also, what would the trait of a spherical cap be then? It is bounding box like but not a box |
That's not relevant for this PR no? There's no Extent for that currently, apart from the approximation -180..180 and 85..90. It's good to get a design/representation for spherical stuff though, like Polygon Full in WKT. |
|
True, just wondering how general the rectangle trait is intended to be - and whether there should be some BoundingBoxTrait or similar. But they need very different handling so you'll probably just have to pass both to some GeometryOps predicate which would then implement the generic behaviour. |
If we have to retro cap packages that sounds very much like a breaking change to me. |
Well, you could argue that Extent not being handled as a geometry is a bug (incomplete implementation). The question is how many downstream packages make use of that non-geometry behaviour (insert relevant xkcd here). Capping makes sense if it's only 1 or 2 packages, making a breaking release requires updating compat for all downstream packages. That said, my next move here is to actually test this behaviour for GO and Rasters, we're only speculating here. |
Fixes #178. Only for the 2D case though, I don't think we can do anything for 3D+, unless we randomly pick the minimum of the added dimensions.