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.

Sign up for GitHub

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

Union bboxes #259

Merged
merged 4 commits into from
Mar 11, 2021
Merged

Union bboxes #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
882fe1e
Union bboxes
jisantuc Mar 9, 2021
09c895d
add a semigroup
jisantuc Mar 10, 2021
ac0b05b
add semigroup tests
jisantuc Mar 10, 2021
e0f2e56
Update changelog
jisantuc Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

### Added
- Added `StacLayer` and `StacLayerProperties` types [#252](https://github.com/azavea/stac4s/pull/252)
- Bboxes can be unioned and form a semigroup [#259](https://github.com/azavea/stac4s/pull/259)

## [0.0.21] - 2021-01-08
### Fixed
Expand Down
13 changes: 13 additions & 0 deletions modules/core-test/js/src/test/scala/com/azavea/stac4s/JsFPLawsSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.azavea.stac4s

import org.scalatest.funsuite.AnyFunSuite
import org.scalatest.matchers.must.Matchers
import org.scalatestplus.scalacheck.Checkers
import org.typelevel.discipline.scalatest.FunSuiteDiscipline

import cats.kernel.laws.discipline.SemigroupTests
import com.azavea.stac4s.testing.TestInstances

class JsFPLawsSpec extends AnyFunSuite with FunSuiteDiscipline with Checkers with Matchers with TestInstances {
checkAll("Semigroup.Bbox", SemigroupTests[Bbox].semigroup)
}
13 changes: 13 additions & 0 deletions modules/core-test/jvm/src/test/scala/com/azavea/stac4s/JvmFPLawsSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.azavea.stac4s

import org.scalatest.funsuite.AnyFunSuite
import org.scalatest.matchers.must.Matchers
import org.scalatestplus.scalacheck.Checkers
import org.typelevel.discipline.scalatest.FunSuiteDiscipline

import cats.kernel.laws.discipline.SemigroupTests
import com.azavea.stac4s.testing.TestInstances

class JvmFPLawsSpec extends AnyFunSuite with FunSuiteDiscipline with Checkers with Matchers with TestInstances {
checkAll("Semigroup.Bbox", SemigroupTests[Bbox].semigroup)
}
1 change: 0 additions & 1 deletion modules/core-test/jvm/src/test/scala/com/azavea/stac4s/JvmSerDeSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,4 @@ class JvmSerDeSpec extends AnyFunSuite with FunSuiteDiscipline with Checkers wit
checkAll("Codec.StacExtent", CodecTests[StacExtent].unserializableCodec)
checkAll("Codec.TemporalExtent", CodecTests[TemporalExtent].unserializableCodec)
checkAll("Codec.StacLayer", CodecTests[StacLayer].unserializableCodec)

}
26 changes: 26 additions & 0 deletions modules/core/js/src/main/scala/com/azavea/stac4s/Bbox.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.azavea.stac4s

import cats.Eq
import cats.kernel.Semigroup
import cats.syntax.either._
import cats.syntax.functor._
import io.circe._
Expand All @@ -12,10 +13,19 @@ sealed trait Bbox {
val xmax: Double
val ymax: Double
val toList: List[Double]

def union(other: Bbox): Bbox
}

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 {
case TwoDimBbox(otherXmin, otherYmin, otherXmax, otherYmax) =>
TwoDimBbox(otherXmin min xmin, otherYmin min ymin, otherXmax max xmax, otherYmax max ymax)
case ThreeDimBbox(otherXmin, otherYmin, zmin, otherXmax, otherYmax, zmax) =>
ThreeDimBbox(otherXmin min xmin, otherYmin min ymin, zmin, otherXmax max xmax, otherYmax max ymax, zmax)
}
}

final case class ThreeDimBbox(
Expand All @@ -27,6 +37,20 @@ final case class ThreeDimBbox(
zmax: Double
) extends Bbox {
val toList = List(xmin, ymin, zmin, xmax, ymax, zmax)

def union(other: Bbox): Bbox = other match {
case TwoDimBbox(otherXmin, otherYmin, otherXmax, otherYmax) =>
ThreeDimBbox(otherXmin min xmin, otherYmin min ymin, zmin, otherXmax max xmax, otherYmax max ymax, zmax)
case ThreeDimBbox(otherXmin, otherYmin, otherZmin, otherXmax, otherYmax, otherZmax) =>
ThreeDimBbox(
otherXmin min xmin,
otherYmin min ymin,
otherZmin min zmin,
otherXmax max xmax,
otherYmax max ymax,
otherZmax max zmax
)
}
}

object TwoDimBbox {
Expand Down Expand Up @@ -83,4 +107,6 @@ object Bbox {

implicit val eqBbox: Eq[Bbox] = Eq.fromUniversalEquals

implicit val semigroupBbox: Semigroup[Bbox] = Semigroup.instance(_ union _)

}
26 changes: 26 additions & 0 deletions modules/core/jvm/src/main/scala/com/azavea/stac4s/Bbox.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.azavea.stac4s

import cats.Eq
import cats.kernel.Semigroup
import cats.syntax.either._
import cats.syntax.functor._
import geotrellis.vector.{Extent, ExtentRangeError}
Expand All @@ -20,10 +21,19 @@ sealed trait Bbox {
} catch {
case e: ExtentRangeError => Either.left(e.toString)
}

def union(other: Bbox): Bbox
}

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@jisantuc jisantuc Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Projection has a bbox field proj:bbox
https://github.com/radiantearth/stac-spec/tree/dev/extensions/projection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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.

Copy link
Contributor Author

@jisantuc jisantuc Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jisantuc jisantuc Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@pomadchin pomadchin Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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):

case TwoDimBbox(otherXmin, otherYmin, otherXmax, otherYmax) =>
TwoDimBbox(otherXmin min xmin, otherYmin min ymin, otherXmax max xmax, otherYmax max ymax)
case ThreeDimBbox(otherXmin, otherYmin, zmin, otherXmax, otherYmax, zmax) =>
ThreeDimBbox(otherXmin min xmin, otherYmin min ymin, zmin, otherXmax max xmax, otherYmax max ymax, zmax)
}
}

final case class ThreeDimBbox(
Expand All @@ -35,6 +45,20 @@ final case class ThreeDimBbox(
zmax: Double
) extends Bbox {
val toList = List(xmin, ymin, zmin, xmax, ymax, zmax)

def union(other: Bbox): Bbox = other match {
case TwoDimBbox(otherXmin, otherYmin, otherXmax, otherYmax) =>
ThreeDimBbox(otherXmin min xmin, otherYmin min ymin, zmin, otherXmax max xmax, otherYmax max ymax, zmax)
case ThreeDimBbox(otherXmin, otherYmin, otherZmin, otherXmax, otherYmax, otherZmax) =>
ThreeDimBbox(
otherXmin min xmin,
otherYmin min ymin,
otherZmin min zmin,
otherXmax max xmax,
otherYmax max ymax,
otherZmax max zmax
)
}
}

object TwoDimBbox {
Expand Down Expand Up @@ -91,4 +115,6 @@ object Bbox {

implicit val eqBbox: Eq[Bbox] = Eq.fromUniversalEquals

implicit val semigroupBbox: Semigroup[Bbox] = Semigroup.instance((_ union _))

}