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

Additional @turf/concave refactoring #907

Merged
merged 15 commits into from
Aug 17, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
16 changes: 10 additions & 6 deletions packages/turf-concave/bench.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ const fixtures = fs.readdirSync(directory).map(filename => {
/**
* Single Process Benchmark
*
* issue-333: 651.884ms
* pts1: 6.568ms
* pts2: 476.032ms
* fiji: 17.973ms
* issue-333: 31.418ms
* pts1: 1.207ms
* pts2: 17.366ms
* pts3: 0.853ms
*/
for (const {name, geojson} of fixtures) {
const {maxEdge, units} = geojson.properties || {maxEdge: 1};
Expand All @@ -29,9 +31,11 @@ for (const {name, geojson} of fixtures) {
/**
* Benchmark Results
*
* issue-333 x 5.57 ops/sec ±10.65% (18 runs sampled)
* pts1 x 315 ops/sec ±3.48% (70 runs sampled)
* pts2 x 4.51 ops/sec ±2.28% (16 runs sampled)
* fiji x 1,513 ops/sec ±10.80% (63 runs sampled)
* issue-333 x 183 ops/sec ±2.41% (74 runs sampled)
* pts1 x 2,934 ops/sec ±2.35% (78 runs sampled)
* pts2 x 136 ops/sec ±3.87% (67 runs sampled)
* pts3 x 7,215 ops/sec ±9.04% (70 runs sampled)
*/
const suite = new Benchmark.Suite('turf-transform-scale');
for (const {name, geojson} of fixtures) {
Expand Down
55 changes: 33 additions & 22 deletions packages/turf-concave/index.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
// 1. run tin on points
// 2. calculate length of all edges and area of all triangles
// 3. remove triangles that fail the max length test
// 4. buffer the results slightly
// 5. merge the results
var tin = require('@turf/tin');
var union = require('@turf/union');
var helpers = require('@turf/helpers');
var distance = require('@turf/distance');
var clone = require('@turf/clone');
var dissolve = require('geojson-dissolve');
var feature = helpers.feature;
var featureCollection = helpers.featureCollection;

/**
* Takes a set of {@link Point|points} and returns a concave hull polygon.
* Takes a set of {@link Point|points} and returns a concave hull Polygon or MultiPolygon.
* Internally, this uses [turf-tin](https://github.com/Turfjs/turf-tin) to generate geometries.
*
* @name concave
Expand All @@ -34,11 +31,16 @@ var clone = require('@turf/clone');
* var addToMap = [points, hull]
*/
module.exports = function (points, maxEdge, units) {
// validation
if (!points) throw new Error('points is required');
if (maxEdge === undefined || maxEdge === null) throw new Error('maxEdge is required');
if (typeof maxEdge !== 'number') throw new Error('invalid maxEdge');

var tinPolys = tin(points);
var cleaned = removeDuplicates(points);

var tinPolys = tin(cleaned);
// calculate length of all edges and area of all triangles
// and remove triangles that fail the max length test
tinPolys.features = tinPolys.features.filter(function (triangle) {
var pt1 = triangle.geometry.coordinates[0][0];
var pt2 = triangle.geometry.coordinates[0][1];
Expand All @@ -51,23 +53,32 @@ module.exports = function (points, maxEdge, units) {

if (tinPolys.features.length < 1) throw new Error('too few polygons found to compute concave hull');

return merge(tinPolys.features);
// merge the adjacent triangles
var dissolved = dissolve(tinPolys.features);
// geojson-dissolve always returns a MultiPolygon
if (dissolved.coordinates.length === 1) {
dissolved.coordinates = dissolved.coordinates[0];
dissolved.type = 'Polygon';
}
return feature(dissolved);
};

/**
* Merges/Unifies all the features in a single polygon
* Removes duplicated points in a collection returning a new collection
*
* @private
* @param {Array<Feature>} features to be merged
* @returns {Feature<(Polygon|MultiPolygon)>} merged result
* @param {FeatureCollection<Point>} points to be cleaned
* @returns {FeatureCollection<Point>} cleaned set of points
*/
function merge(features) {
var merged = clone(features[0]);
merged.properties = {};

for (var i = 0, len = features.length; i < len; i++) {
var poly = features[i];
if (poly.geometry) merged = union(merged, poly);
}
return merged;
function removeDuplicates(points) {
var cleaned = [];
var existing = {};
points.features.forEach(function (pt) {
Copy link
Member

Choose a reason for hiding this comment

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

👍 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! I told you it was sooo smart! 😄

var key = pt.geometry.coordinates.join('-');
if (!existing.hasOwnProperty(key)) {
cleaned.push(pt);
existing[key] = true;
}
});
return featureCollection(cleaned, points.properties);
Copy link
Member

Choose a reason for hiding this comment

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

FeatureCollection doesn't have properties as a param.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah... ok! 👍

}
18 changes: 12 additions & 6 deletions packages/turf-concave/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,31 @@
],
"author": "Turf Authors",
"contributors": [
"Stefano Borghi <@stebogit>"
"Tom MacWright <@tmcw>",
"Lyzi Diamond <@lyzidiamond>",
"Denis Carriere <@DenisCarriere>",
"Stefano Borghi <@stebogit>",
"Rowan Winsemius <@rowanwins>",
"Daniel Pulido <@dpmcmlxxvi>",
"Stephen Whitmore <@noffle>",
"Gregor MacLennan <@gmaclennan>"
],
Copy link
Member

Choose a reason for hiding this comment

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

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

"license": "MIT",
"bugs": {
"url": "https://github.com/Turfjs/turf/issues"
},
"homepage": "https://github.com/Turfjs/turf",
"devDependencies": {
"@turf/helpers": "^4.6.0",
"@turf/meta": "^4.6.0",
"benchmark": "^2.1.4",
"load-json-file": "^2.0.0",
"tape": "^4.6.3",
"write-json-file": "^2.1.4"
},
"dependencies": {
"@turf/distance": "^4.6.0",
"@turf/tin": "^4.6.0",
"@turf/union": "^4.6.0",
"@turf/clone": "^4.6.0"
"@turf/distance": "4.6.0",
"@turf/helpers": "^4.6.0",
"@turf/tin": "4.6.0",
"geojson-dissolve": "3.1.0"
}
}
7 changes: 4 additions & 3 deletions packages/turf-concave/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ const fixtures = fs.readdirSync(directories.in).map(filename => {
};
});

test('turf-line-split', t => {
test('turf-concave', t => {
for(const {filename, name, geojson} of fixtures) {
const {maxEdge, units} = geojson.properties || {maxEdge: 1};
let {maxEdge, units} = geojson.properties;
maxEdge = maxEdge || 1;
const hull = concave(geojson, maxEdge, units);
featureEach(geojson, stylePt);
const results = featureCollection([...geojson.features, hull]);
Expand All @@ -37,7 +38,7 @@ test('turf-line-split', t => {
const points = featureCollection([point([0, 0]), point([1, 1]), point([1, 0])]);
const onePoint = featureCollection([point([0, 0])]);

test('concave', t => {
test('concave -- throw', t => {
t.throws(() => concave(onePoint, 5.5, 'miles'), /too few polygons found to compute concave hull/, 'too few points');
t.throws(() => concave(onePoint, 0), /too few polygons found to compute concave hull/, 'maxEdge too small');

Expand Down
Loading