Union bboxes #259
Union bboxes #259
Conversation
|
The pitfall with making a semigroup is what would this semigroup mean? Is this semigroup always a union? Motivating example: https://github.com/geotrellis/geotrellis-server/blob/develop/stac-example/src/main/scala/geotrellis/server/ogc/stac/SearchFiltersQuery.scala#L144-L145 EDIT: we talked: union is the only valid semigroup here, we can't have empty bboxes. |
| } | ||
|
|
||
| final case class TwoDimBbox(xmin: Double, ymin: Double, xmax: Double, ymax: Double) extends Bbox { | ||
| val toList = List(xmin, ymin, xmax, ymax) | ||
|
|
||
| def union(other: Bbox): Bbox = other match { |
There was a problem hiding this comment.
I believe this will give the incorrect results if (at least) one of the bboxes crosses the antimeridian, e.g., [178.0, 0.0, -178.0, 1.0] ∪ [177.0, 0.0, 178.0, 1.0] should be [177.0, 0.0, -178.0, 1.0] instead of [177.0, 0.0, 178.0, 1.0] (-178 is the most northeasterly point of the extent rather than the max value of 178)
it might also be useful to name the attributes like (sw|ne)_(lon|lat) instead of (x|y)(min|max)
There was a problem hiding this comment.
Oof that's a fair point. On the one hand there's nothing here that says this is lat/long only, on the other hand it's a special type to hold bboxes from geojson which should always be lat/long, and on the third hand there's the projection extension which lives in core. Do you know off-hand whether there are many existing catalogs that implement extension? There's the example linked in the extension itself, but stacindex doesn't have much. If that extension weren't in core I'd say it's a user problem but its being in core makes me feel like we should at least make an explicit decision about it, even if that decision is "user-land problem".
There was a problem hiding this comment.
How is that connected to the projection extension? It has its own set of fields and none of them is the Bbox.
I had a feeling that it only adds these proj fields and new geometry fields can already be in other than LatLon projections.
P.S. for consisteny reasons (with GT) I prefer x / y / min / max ¯_(ツ)_/¯ I always forget the lat / lon / sw / ne ordering.
There was a problem hiding this comment.
Maybe we have two different BBox types? One is locked to WGS 84 and used for the Feature/Item bbox and has union, and the other (used for proj:bbox) is just an abstraction of 2 or 3 points that (I don't think) can have a generalized union operation -- the killer argument there is probably that 2 BBoxs don't necessarily have to be in the same CRS. I have no idea how polar projections work either, but seems like that would be another edge case.
I originally created the projection extension to standardize the native geometry fields used by Astraea EOD, but bbox was added to the spec later by someone else, so EOD doesn't use it.
There was a problem hiding this comment.
How is that connected to the projection extension? It has its own set of fields and none of them is the Bbox.
I had a feeling that it only adds theseprojfields and new geometry fields can already be in other than LatLon projections.
Projection has a bbox field proj:bbox
https://github.com/radiantearth/stac-spec/tree/dev/extensions/projection
There was a problem hiding this comment.
I think we're going to have a lot of trouble getting consistent anti-meridian handling.
I did some experiments with shapely, turf, and jts. I created boxes with xmin = 179, xmax = -179, ymin = 0, ymax = 2 with each of them.
With shapely and jts, the library helpfully flips the x min / max for you and you end up with this guy:
With turf there's no box helper, but I created a polygon with those coordinates, which worked great:
> const { polygon } = require("@turf/helpers");
undefined
> polygon([[[179, 0], [-179, 0], [-179, 2], [179, 2], [179, 0]]], {})
{
type: 'Feature',
properties: {},
geometry: { type: 'Polygon', coordinates: [ [Array] ] }
}
> JSON.stringify(_)
'{"type":"Feature","properties":{},"geometry":{"type":"Polygon","coordinates":[[[179,0],[-179,0],[-179,2],[179,2],[179,0]]]}}'except neither geojson.io nor qgis thinks that's a valid feature / geometry.
I think the choice for anti-meridian handling is between exceeding the lat/long bounds (e.g., -179 gets rewritten to 181 if the xmin is 179) and noting that bbox unions when interpreted as lat/long can produce surprising bboxes while steering people toward preferring the geometry field whenever they have a choice.
There was a problem hiding this comment.
Oops, I entered the turf feature incorrectly into geojson.io the first time -- it displays it, but not surprisingly I guess it also displays as the huge equatorial rectangle
There was a problem hiding this comment.
I would avoid comparing this against the geometry behavior in those libraries -- I think a lot of those tools make the (almost always reasonable, but wrong) assumption that antimeridian-spanning polygons are just wound the wrong way and are supposed to be the "horizontal" inverse. I do think it's an odd behavior to just take the min and max of the two bbox points, since it's unlikely those would be flipped.
But, I think it's reasonable to expect that if I have an antimeridian spanning bbox and union it with something else, I get an antimeridian spanning bbox back. The edge case (literally!) of two bboxes that align on opposite sides of the antimeridan should probably give back a -180 to 180 bbox instead of an antimeridian spanning one. In other cases where an antimeridian-spanning bbox is smaller than the non-antimeridian-spanning one (e.g., one scene in Australia and one in Alaska), I wouldn't necessarily expect to get back the smallest bbox and/or one that spanned the antimeridian, so a huge non-antimeridian-spanning one covering most of the globe would be reasonable.
But, overall, whatever the semantics are for union, the resulting bbox should actually cover the two input bboxes.
There was a problem hiding this comment.
But, overall, whatever the semantics are for union, the resulting bbox should actually cover the two input bboxes.
That's what I'm proposing to accomplish with e.g. rewriting -179 to 181. What I'm especially interested in avoiding is:
- errors on conversion to
Extents in GeoTrellis, which we'll get if we keep the smaller max than min - surprising conversions when we're totally happy with a bbox, but someone is reading an item with PySTAC or whatever and shapely decides it's time to help out
I want to scope down what I'm responsible for here to:
- non-spatially aware unions of bboxes should cover both bboxes
That is what's required for azavea/franklin#623, which is the problem this PR is trying to solve. I propose solving that by being naive/trusting about what min and max mean and sticking with the very basic implementation that's already here.
I want to defer responsibility for:
- 2d bbox should be isomorphic to extent (violated if bboxes are allowed to have xmax < xmin)
- bboxes should understand how special 180 / -180 are and choose unioning accordingly
I think the central tension is that bboxes in stac4s don't know anything about projections, but our notion of "covers" here is aware that these coordinates are on a globe.
If GT supported 3d Extents and ProjectedExtents we could re-use those and solve this problem there.
There was a problem hiding this comment.
@jisantuc 👍 all makes sense to me; I will post here issues that came to mind after this conversation happened (leaving it here for the history purposes so we'd remember having this covnersation):
real annoying to have to duplicate 🤔, since the type doesn't exist in shared 🤷🏻♂️
Overview
This PR makes bboxes unionable. I don't know why I didn't give them a semigroup. I should just give them a semigroup. I'll do that tomorrow. 🤦🏻♂️
A release including this PR blocks azavea/franklin#623
Checklist
chan)Notes
Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.
Closes #XXX (if applicable)