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

Repaired non-robust Delaunay triangulator. #1699

Merged
merged 6 commits into from Nov 2, 2016

Conversation

Projects
None yet
2 participants
@jpolchlo
Contributor

jpolchlo commented Oct 25, 2016

This PR replaces the custom, non-robust Delaunay triangulator with wrapped version of the JTS triangulator. The old triangulator did not implement robust predicates for collinearity/cocircularity which could cause failures in some real-world data sets.

The JTS triangulator was wrapped to allow a more idiomatic Scala usage. Namely, allowing for PointFeatures to be used more easily, which would facilitate the use of a triangulation for the purposes of, say, interpolating a field of numbers defined over a sample of points. This wrapped version also replaces the quad-edge structure used by JTS for a lighter-weight half-edge structure. Documentation for that structure can be found in docs/vector/voronoi.md.

This PR closes issue #1683.

@lossyrob

This comment has been minimized.

Member

lossyrob commented Oct 25, 2016

@jpolchlo you made benchmarks for the old voronio, right? Is it possible to rerun those benchmarks, against the two different versions, to see if we're losing a lot of speed?

@jpolchlo

This comment has been minimized.

Contributor

jpolchlo commented Oct 25, 2016

You and I sat down to look at benchmarks, but I didn't write them myself,
so I'm not 100% on how to proceed. With some pointers, I'm sure I could do
it. But I wonder what you have in mind here, since the version that got
replaced will fail on a certain class of inputs, and surely, if I were to
make my original code robust, it will necessarily slow down based on the
additional operations needed to ensure correctness.

On Tue, Oct 25, 2016 at 11:08 AM, Rob Emanuele notifications@github.com
wrote:

@jpolchlo https://github.com/jpolchlo you made benchmarks for the old
voronio, right? Is it possible to rerun those benchmarks, against the two
different versions, to see if we're losing a lot of speed?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1699 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ARjt0aoZ-AZkd9adAEvp0GmdZEsPb57Dks5q3htngaJpZM4KgAI8
.

@lossyrob

This comment has been minimized.

Member

lossyrob commented Oct 26, 2016

@jpolchlo the idea is to have a sense of how much things have slowed down. And to have an idea of how efficient this operation is...I remember that you were calculating triangulations on the nickel dataset in about 200ms. If this turns out to be a 2x vs a 10x slowdown, that would be good to understand. If it's a serious slowdown, I'd want to either try to optimize before release or at least put in an issue for future optimization.

@jpolchlo

This comment has been minimized.

Contributor

jpolchlo commented Oct 26, 2016

Roger, boss. I'll see what I can figure out.

On Tue, Oct 25, 2016 at 10:47 PM, Rob Emanuele notifications@github.com
wrote:

@jpolchlo https://github.com/jpolchlo the idea is to have a sense of
how much things have slowed down. And to have an idea of how efficient this
operation is...I remember that you were calculating complex triangulations
on the nickel dataset in about 200ms. If this turns out to be a 2x vs a 10x
slowdown, that would be good to understand. If it's a serious slowdown, I'd
want to either try to optimize before release or at least put in an issue
for future optimization.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1699 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ARjt0WyJPoijxi0-UCG5rQR9NBuSpUBeks5q3r9EgaJpZM4KgAI8
.

@lossyrob lossyrob added this to the 1.0 milestone Oct 26, 2016

@jbouffard jbouffard force-pushed the locationtech:master branch from 64b2f0c to f1c58f9 Oct 28, 2016

val cm = ColorMap(scala.collection.immutable.Map(1 -> 0x000000ff, 255 -> 0xffffffff))
tile.renderPng(cm).write("delaunay.png")
}
*/

This comment has been minimized.

@lossyrob

lossyrob Oct 30, 2016

Member

What's going on with all the commented code here?

(dt.triangles.forall{ case ((ai,bi,ci),_) =>
val otherPts = (0 until numpts).filter{ i: Int => i != ai && i != bi && i != ci }
otherPts.forall{ i => ! localInCircle((ai,bi,ci),i) }
}) should be (true)
}
it("should work on a real dataset with many collinear points") {
val allcities = ResourceReader.asString("populous_cities.txt")

This comment has been minimized.

@lossyrob

lossyrob Oct 30, 2016

Member

We don't normally read test data out of resources packed in the JAR - is there an advantage you see of doing it this way, other then adding to test files similar to other tests? We would put it here https://github.com/geotrellis/geotrellis/tree/master/vector-test/data

import java.io._
object ResourceReader {
def asString(filename: String): String = {

This comment has been minimized.

@lossyrob

lossyrob Oct 30, 2016

Member

This is for test functionality, not sure it's worth putting in the core utils JAR

This comment has been minimized.

@jpolchlo

jpolchlo Oct 31, 2016

Contributor

Had talked to Eugene about this. He suggested putting it in util, but it's not super valuable, so I'll nuke it.

* boundary(.next)*.face == None.
*/
val boundary = triangulate(0, vIx.length-1)
val triangles = _triangles.map{ poly => Polygon.jtsToPolygon(poly.asInstanceOf[JTSPolygon]) }

This comment has been minimized.

@lossyrob

lossyrob Nov 1, 2016

Member

can just use Polygon(poly.asInstanceOf[JTSPolygon]

import org.scalatest.{FunSpec, Matchers}
class DelaunaySpec extends FunSpec with Matchers {

This comment has been minimized.

@lossyrob

lossyrob Nov 1, 2016

Member

Should we really scrap all these tests? I think still testing our JTS backed Delaunay class makes sense.

This comment has been minimized.

@jpolchlo

jpolchlo Nov 1, 2016

Contributor

As much as I would like to provide some unit tests here, I'm just at a loss for what should be tested. It seems that most anything that I could test for tests JTS's feature set, and not the features of this package. I'm not sure if that's appropriate. As it is, there is an indirect test of Delaunay and Voronoi in the EuclideanDistanceTileSpec. I'd be happy for some suggestions about what should be tested for in these classes!

This comment has been minimized.

@jpolchlo

jpolchlo Nov 1, 2016

Contributor

After some discussion, we've opted to re-insert some tests essentially as regression tests.

}
private val _triangles = {
val tris = subd.getTriangles(gf)
for ( i <- 0 until tris.getNumGeometries ) yield tris.getGeometryN(i)

This comment has been minimized.

@lossyrob

lossyrob Nov 1, 2016

Member

use a cfor loop for performance...also do we need the private val?

import spire.syntax.cfor._

val triangles: Seq[Polygon] = {
  val arr = Array.ofDim[Polygon](tris.getNumGeometries)
  val len = arr.length
   cfor(0)(_ < len, _ + 1) { i =>
    arr(i) = Polygon(tris.getGeometryN(i).asInstanceOf[JTSPolygon])
  }
  arr
}

jpolchlo added some commits Nov 1, 2016

@lossyrob lossyrob merged commit 5ca4a8b into locationtech:master Nov 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lossyrob

This comment has been minimized.

Member

lossyrob commented Nov 2, 2016

@jpolchlo "This PR closes issue #1683." doesn't kick off the auto close. Has to be "Fixes #1683".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment