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.

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

Initial commit remove jsts for turf/difference #1225

Merged
merged 7 commits into from
Jan 27, 2018

Conversation

rowanwins
Copy link
Member

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

Well I thought I'd make the much anticipated start of removing jsts from turf using martinez.

The initial commit covers turf/difference only.

So far

  • Tests look good except for create-hole, this is a known issue in Martinez at the moment,I'll try and dig into this some more.

Benchmarks

Generally martinez is smashing with jsts although there are a few exceptions around issue 721 for some reason...

 * ==using jsts==
 * clip-polygons x 2,512 ops/sec ±30.29% (71 runs sampled)
 * completely-overlapped x 6,777 ops/sec ±4.08% (78 runs sampled)
 * create-hole x 5,451 ops/sec ±9.92% (75 runs sampled)
 * issue-#721-inverse x 434,372 ops/sec ±3.40% (85 runs sampled)
 * issue-#721 x 421,194 ops/sec ±4.35% (80 runs sampled)
 * multi-polygon-input x 1,904 ops/sec ±5.16% (79 runs sampled)
 * multi-polygon-target x 1,240 ops/sec ±5.44% (79 runs sampled)
 * split-polygon x 2,468 ops/sec ±4.75% (82 runs sampled)

 * ==using martinez==
 * clip-polygons x 38,724 ops/sec ±11.98% (65 runs sampled)
 * completely-overlapped x 70,644 ops/sec ±8.29% (67 runs sampled)
 * create-hole x 62,128 ops/sec ±7.45% (75 runs sampled)
 * issue-#721-inverse x 354,473 ops/sec ±2.46% (81 runs sampled)
 * issue-#721 x 339,053 ops/sec ±3.24% (78 runs sampled)
 * multi-polygon-input x 17,728 ops/sec ±10.01% (64 runs sampled)
 * multi-polygon-target x 14,077 ops/sec ±6.52% (75 runs sampled)
 * split-polygon x 29,258 ops/sec ±8.99% (69 runs sampled)

Anyway off to pretty good start I think thanks to the incredible work by @w8r

@DenisCarriere
Copy link
Member

🎉 Woot! Those benchmarks look amazing! Great job @rowanwins implemented this and thanks @w8r for the work on Martinez

@@ -43,22 +43,16 @@ function difference(polygon1, polygon2) {
var geom2 = getGeom(polygon2);
var properties = polygon1.properties || {};

// Issue #721 - JSTS can't handle empty polygons
// Issue #721 - JSTS/Martinez can't handle empty polygons
Copy link
Member

Choose a reason for hiding this comment

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

😢 Same issue is occuring with Martinez?...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes the issue still occurs in Martinez but I didn't dig into it too deeply at 11pm so I'll look into it further

@@ -7,50 +7,54 @@
"fill-opacity": 1
},
"geometry": {
"type": "Polygon",
"type": "MultiPolygon",
Copy link
Member

Choose a reason for hiding this comment

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

@rowanwins 🤔 ... you sure this is correct?... That was the initial issue was trying to handle the holes.

Copy link
Member

@DenisCarriere DenisCarriere Jan 17, 2018

Choose a reason for hiding this comment

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

Seems like the Inner blue should be a lighter shade.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct - so martinez currently has an issue around this, I had a thought on how to resolve it last night lying in bed so I'll try and tackle it today

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI I have a pull request in to resolve this issue upstream in martinez

@DenisCarriere
Copy link
Member

@rowanwins Going to put this PR on the back burner until your PR w8r/martinez#55 gets resolved/merged.

@w8r
Copy link

w8r commented Jan 25, 2018

Just merged w8r/martinez#55 and released on npm

@DenisCarriere
Copy link
Member

👍 Great Thanks @w8r I'll update this PR with your new changes to Martinez.

@mfogel
Copy link

mfogel commented Jan 25, 2018

Edit: This appears to be a problem internal to martinez, part of w8r/martinez#35

I was checking out to see if this branch fixes #1224 and it appears that right now, this branch still has problems with the situation described by #1224. Here's a test case that can be dropped into turf-difference/test/in that causes node to crash with a heap-out-of-memory error (which I think is probably runaway recursion but I haven't verified that).

{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "type": "Polygon",
        "coordinates": [[[-1, -10], [0, -10], [0, 10], [-1, 10], [-1, -10]]]
      }
    },
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "type": "MultiPolygon",
        "coordinates": [
          [[[-1, -10], [0, -10], [0, 10], [-1, 10], [-1, -10]]],
          [[[0, -10], [1, -10], [1, 10], [0, 10], [0, -10]]]
        ]
      }
    }
  ]
}

@rowanwins
Copy link
Member Author

Thanks for the research and test case @mfogel - we'll take a look and see what we can do

@DenisCarriere
Copy link
Member

👍 Looks like it's working now.

Going to publish this under v6.0

CC: @rowanwins @w8r

image

Copy link
Member

@DenisCarriere DenisCarriere left a comment

Choose a reason for hiding this comment

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

👍

@DenisCarriere DenisCarriere merged commit 0df5a20 into master Jan 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants