Additional `@turf/concave` refactoring #907

stebogit · 2017-08-17T08:45:09Z

Following #899:

replaced @turf/union with geojson-dissolve, thus removed jsts dependency 🎊
⬆️ ⬆️ ⬆️ ⬆️ speed 👍
Support null geometries

The reason why I preferred geojson-dissolve to concaveman (as previously suggested) is because the latter returns always a single concave Polygon embracing all the input points, while geojson-dissolve allows to return (Multi)Polygons even with holes if appropriate (as the current @turf/concave implementation does).
However, for what I've seen online after some research, I'm not sure that this is the right/expected behaviour of a concave module. This is mainly due to the maxEdge parameter: if it was set to Infinity @turf/concave would always return a single concave hull Polygon (like concaveman).
In the next major release I would set maxEdge to Infinity by default.

Also I noticed this module throws an Error if there is no hull to return ("too few polygons found to compute concave hull"), likely due to a small maxEdge. I would rather return an empty Feature.

- replaced turf-union with geojson-dissolve; - updated and added tests; - updated bench and readme;

rowanwins · 2017-08-17T12:30:50Z

Hey @stebogit

That geojson-dissolve module looks interesting, a few q's,

what is performance of it like compared to our dissolve module
whats is it like size wise once it includes its dependencies

Im sure either the geojson-dissolve or concaveman approaches will be a big improvement!

DenisCarriere · 2017-08-17T15:03:00Z

thus removed jsts dependency

🎉 Woot! 👍

while geojson-dissolve allows to return (Multi)Polygons even with holes if appropriate (as the current @turf/concave implementation does).

👍 Better to keep it similar to the same implementation, if one wants to use concaveman then it's pretty straightforward to use as an external library.

In the next major release I would set maxEdge to Infinity by default.

👍 Sounds like a plan!

Also I noticed this module throws an Error if there is no hull to return ("too few polygons found to compute concave hull"), likely due to a small maxEdge. I would rather return an empty Feature.

👍 Sounds good, this shouldn't be an error. An error should be caused when you are misusing the module (which isn't the case).

Question/Proposal (not immediate to this PR)

Improve/refactor @turf/dissolve using geojson-dissolve
Drop @turf/union in next major release in favor of using @turf/dissolve (both modules serve the same purpose).

If we are replacing @turf/union with geojson-dissolve, couldn't we refactor @turf/dissolve to use that module (currently it's also using @turf/union).

Also, what's the difference between @turf/union & @turf/dissolve, seems like they would serve the same purpose (@turf/dissolve has a bit more features, but it it's essentially the same for the geometry).

DenisCarriere

👍 I'm really liking geojson-dissolve, pretty neat library!

DenisCarriere · 2017-08-17T16:18:48Z

packages/turf-concave/package.json

+    "Denis Carriere <@DenisCarriere>",
+    "Stefano Borghi <@stebogit>",
+    "Rowan Winsemius <@rowanwins>",
+    "Daniel Pulido <@dpmcmlxxvi>"
  ],


I would also include the Authors/Contributors of geojson-dissolve.

Stephen Whitmore @noffle

Gregor MacLennan @gmaclennan

hackergrrl · 2017-08-17T18:15:36Z

Nice, someone else is using our code. ❤️

It's worth noting that geojson-dissolve is just a thin wrapper around topojson for the Polygon case, and deserve lots of credit for doing the Hard Work.

DenisCarriere · 2017-08-17T18:17:49Z

packages/turf-concave/index.js

+function removeDuplicates(points) {
+    var cleaned = [];
+    var existing = {};
+    points.features.forEach(function (pt) {


👍 This syntax looked so familiar that I thought I wrote this part :)

https://github.com/Turfjs/turf/blob/master/packages/turf-clean-coords/index.js#L52-L58

Yes! I told you it was sooo smart! 😄

Also include Polygon or MultiPolygon as valid output

DenisCarriere · 2017-08-17T18:32:57Z

@noffle You're code is awesome! 👍 You've got a new fan boy for you're many awesome repos in Digital Democracy.

thin wrapper around topojson for the Polygon case, and deserve lots of credit for doing the Hard Work.

We will add Mike Bostock @mbostock to the list of contributors for his great work in topojson.

DenisCarriere · 2017-08-17T19:04:31Z

@rowanwins what is performance of it like compared to our dissolve module

Humm.. I personally worked on @turf/dissolve, one of the main differences is the use of rbush to quickly create an index of the geometries that intersects each other and only perform the union/dissolve operation on those geometries.

Looking at the source code of geojson-dissolve there's a few optimizing things that can speed up the performance:

flattenEach in @turf/meta (instead of using flatten + geomEach)
@turf/clone instead of using JSON.parse + JSON.stringify
geojson-rbush to reduce the amount of dissolve operations (GeoJSON => TopoJSON => Dissolve => TopoJSON => GeoJSON).
Would it be possible to translate topojson.merge to pure GeoJSON? It would be worth doing for performance, but might take some time to figure out 🤔..

DenisCarriere · 2017-08-17T19:08:18Z

packages/turf-concave/index.js

+            existing[key] = true;
+        }
+    });
+    return featureCollection(cleaned, points.properties);


FeatureCollection doesn't have properties as a param.

ah... ok! 👍

hackergrrl · 2017-08-17T19:21:03Z

Very happy to accept those PRs. 😁

DenisCarriere · 2017-08-17T19:59:21Z

Very happy to accept those PRs.

Me too! 👍 haha

DenisCarriere · 2017-08-17T21:37:45Z

+10 commits later, I think this last commit will "pass" lol 😫

rowanwins · 2017-08-17T22:41:56Z

Gday @DenisCarriere

I wrote the bulk of the dissolve module so yep I remember using rbush :)

I'd think it's worth keeping both union and dissolve. While geometry-wise they might have similar inputs and outputs their application is a bit different.

In regards to geojson-dissolve it would be good to do some performance testing against our existing dissolve module. It would be good to see how performant it is in all those conversions of format etc.

DenisCarriere · 2017-08-17T22:46:55Z

@rowanwins 👍 For sure, we can do that on a separate PR, I'm also interested in seeing how jsts performs compared to topojson.merge.

DenisCarriere · 2017-08-17T22:47:22Z

I wrote the bulk of the dissolve module so yep I remember using rbush :)

Yes team work 👍 🥇

stebogit · 2017-08-18T06:48:18Z

@rowanwins @DenisCarriere geojson-dissolve merges only contiguous features, i.e. sharing a side or segment -- being it based on TopoJSON it make sense. If the features don't touch or overlap it just returns the original inputs .
Thus it was perfect for @turf/concave, which uses @turf/tin, but I don't see it particularly useful for @turf/union or @turf/dissolve.
I've been searching online for union/merge algorithms and tools to replace jsts, but no luck so far... 😞
But we'll find a way, eventually! 😃

stebogit · 2017-08-18T07:40:30Z

Drop @turf/union in next major release in favor of using @turf/dissolve (both modules serve the same purpose).

...and dissolve does not use jsts! 👍
However I'd still call the final module @turf/union, a quite common term for boolean operations on polygons, and I'd define the propertyName (now a @turf/dissolve argument) to optional, merging all the features by default.

stebogit · 2017-08-20T07:53:36Z

I'd think it's worth keeping both union and dissolve. While geometry-wise they might have similar inputs and outputs their application is a bit different.

@rowanwins what kind of differences do you have in mind? It looks to me they could be integrated in a single module that can handle a "filter/selection" parameter.

rowanwins · 2017-08-21T00:17:35Z

My understanding is that traditionally the output of a union operation won't have any overlapping features, whereas with a dissolve you can still have overlapping features.

This is a helpful article, by and large I think it would be good to retain consistent language with what everyone else is doing.

DenisCarriere · 2017-08-21T03:10:40Z

👍 Thanks for the reference @rowanwins.

Helpful link, got me thinking of a few new easy modules @turf/merge & @turf/append (we can open up new issues for that - if you feel those are useful).

I understand a bit better now the difference between dissolve/union, the main difference with dissolve is spatial operation based on a common attribute (which it's currently doing 👍 ).

stebogit · 2017-08-25T08:04:26Z

the output of a union operation won't have any overlapping features, whereas with a dissolve you can still have overlapping features.

@rowanwins so basically dissolve is a "filtered" union on the collection of features?
We might have one single module that accept an optional filter function/parameter that define which feature should be merged. If well thought the filter could actually give much more flexibility to the module, and so to the user.

DenisCarriere · 2017-08-29T04:37:57Z

👍 @turf/filter module, I also did this type of filter behavior in the @turf/cluster module, but it would better if this was extracted out of the clusters module and reside in it's own repo.

stebogit added 4 commits August 17, 2017 01:06

additional refactoring of turf-concave:

b753f16

- replaced turf-union with geojson-dissolve; - updated and added tests; - updated bench and readme;

Merge branch 'master' into concaveman

93c66f2

added hole test

f7885be

added concave-hull test

1abc084

stebogit added enhancement refactoring remove-jsts labels Aug 17, 2017

stebogit self-assigned this Aug 17, 2017

stebogit requested a review from DenisCarriere August 17, 2017 08:45

stebogit added the waiting-review label Aug 17, 2017

DenisCarriere approved these changes Aug 17, 2017

View reviewed changes

DenisCarriere reviewed Aug 17, 2017

View reviewed changes

Add contributors of geojson-dissolve

1d4bc35

DenisCarriere reviewed Aug 17, 2017

View reviewed changes

Add Typescript Units from @turf/helpers

3abf424

Also include Polygon or MultiPolygon as valid output

DenisCarriere added 2 commits August 17, 2017 14:35

Update benchmark & add Mike to contributors

5a67910

Remove dbscan from testing

8542491

Merge branch 'master' into concave-fix

2354cd9

DenisCarriere reviewed Aug 17, 2017

View reviewed changes

DenisCarriere added 2 commits August 17, 2017 15:09

Remove properties from featureCollection method

c5d204a

Update readme

6aec615

DenisCarriere added 2 commits August 17, 2017 16:05

truncate @turf/simplified results (?)

eb30104

Support for null geometries

bd2720a

DenisCarriere added 2 commits August 17, 2017 17:34

Merge branch 'master' into concave-fix

97647e0

Fix test/out for @turf/simplify

f41d202

DenisCarriere merged commit b8f62e6 into master Aug 17, 2017

DenisCarriere deleted the concave-fix branch August 17, 2017 22:47

stebogit mentioned this pull request Aug 20, 2017

@turf/concave improvements #910

Closed

2 tasks

DenisCarriere mentioned this pull request Aug 29, 2017

New module proposal @turf/filter #921

Closed

stebogit added this to the 4.7.0 milestone Sep 3, 2017

DenisCarriere mentioned this pull request Sep 5, 2017

New minor release! Turf 4.7.1 🎉 #932

Closed

stebogit mentioned this pull request Apr 14, 2020

Question about additional functionality related to lineToPolygon function #1881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional `@turf/concave` refactoring #907

Additional `@turf/concave` refactoring #907

stebogit commented Aug 17, 2017 •

edited

Loading

rowanwins commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017 •

edited

Loading

DenisCarriere left a comment

DenisCarriere Aug 17, 2017

hackergrrl commented Aug 17, 2017

DenisCarriere Aug 17, 2017

stebogit Aug 18, 2017

DenisCarriere commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

DenisCarriere Aug 17, 2017

stebogit Aug 18, 2017

hackergrrl commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

rowanwins commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

stebogit commented Aug 18, 2017

stebogit commented Aug 18, 2017 •

edited

Loading

stebogit commented Aug 20, 2017

rowanwins commented Aug 21, 2017

DenisCarriere commented Aug 21, 2017

stebogit commented Aug 25, 2017 •

edited

Loading

DenisCarriere commented Aug 29, 2017

Additional @turf/concave refactoring #907

Additional @turf/concave refactoring #907

Conversation

stebogit commented Aug 17, 2017 • edited Loading

rowanwins commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017 • edited Loading

Question/Proposal (not immediate to this PR)

DenisCarriere left a comment

Choose a reason for hiding this comment

DenisCarriere Aug 17, 2017

Choose a reason for hiding this comment

hackergrrl commented Aug 17, 2017

DenisCarriere Aug 17, 2017

Choose a reason for hiding this comment

stebogit Aug 18, 2017

Choose a reason for hiding this comment

DenisCarriere commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

DenisCarriere Aug 17, 2017

Choose a reason for hiding this comment

stebogit Aug 18, 2017

Choose a reason for hiding this comment

hackergrrl commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

rowanwins commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

DenisCarriere commented Aug 17, 2017

stebogit commented Aug 18, 2017

stebogit commented Aug 18, 2017 • edited Loading

stebogit commented Aug 20, 2017

rowanwins commented Aug 21, 2017

DenisCarriere commented Aug 21, 2017

stebogit commented Aug 25, 2017 • edited Loading

DenisCarriere commented Aug 29, 2017

Additional `@turf/concave` refactoring #907

Additional `@turf/concave` refactoring #907

stebogit commented Aug 17, 2017 •

edited

Loading

DenisCarriere commented Aug 17, 2017 •

edited

Loading

stebogit commented Aug 18, 2017 •

edited

Loading

stebogit commented Aug 25, 2017 •

edited

Loading