New @turf/isolines based on MarchingSquares.js #781

stebogit · 2017-06-07T16:08:42Z

Fixes #390

DenisCarriere · 2017-06-07T17:39:54Z

?? Why all the changes to turf-bbox & turf-helpers??

Looks like you merged an old branch to this PR, might be best to close this PR and only send the changes for @turf/isolines.

👎

stebogit · 2017-06-07T18:08:59Z

@DenisCarriere Wow man, I didn't see those! 😳
It seems actually that it was my last yarn update (all I did was running yarn in my terminal) which generated all those changes in turf-bbox and turf-helpers; everything before was ok.
Shouldn't have I done that maybe?

DenisCarriere · 2017-06-07T18:16:28Z

Humm.. strange, I don't think yarn would of caused that.

Who knows!

I've been starting to use GitHub Desktop which allows you to easily Update from Default Branch. That will make your "stale" branch current to the Master branch.

This might not work in this current state, but that's what I did for some other older branches.

stebogit · 2017-06-07T18:22:53Z

@DenisCarriere you can easily see that those changes occurred only in the last commit; and I swear in there I only ran yarn cause I thought it would have updated the yard.lock file (which it did, among the other changes).
And you can see from the chart I branched out from the HEAD of master.

However do you want me to close this PR and copy all the good changes on a new branch? I believe though we might just undo the last commit and all should be fine.

DenisCarriere · 2017-06-07T18:26:12Z

Undo the commit works. 👍

DenisCarriere · 2017-06-07T18:27:08Z

That's really strange... Ah well... no worries 👍

stebogit · 2017-06-07T18:36:52Z

So, for the next time, should I run yarn to update the yarn.lock file or it get's updated maybe at merge or when Travis runs?

DenisCarriere · 2017-06-07T18:59:48Z

You can still run yarn, but just keep track of your git commit & git push files.

stebogit · 2017-06-08T03:45:33Z

Yay!! Undo done 😃

DenisCarriere · 2017-06-08T04:36:03Z

packages/turf-isolines/index.d.ts


 /**
 * http://turfjs.org/docs/#isolines
 */
-declare function isolines(points: Points, z: string, resolution: number, breaks: Array<number>): LineStrings;
+declare function isolines(points: Points, breaks: Array<number>, property?: string): MultiLineStrings;


Looks like you're missing options

You would need to build an interface for that.

interface Options { isolineProperties: any[], commonProperties: any }

Then declare the option as a param.

DenisCarriere · 2017-06-08T04:38:06Z

packages/turf-isolines/index.js

+ * @param {Object} [options={}] options on output
+ * @param {Array<Object>} [options.isolineProperties=[]] GeoJSON properties passed, in order, to the correspondent
+ * isoline (order defined by breaks)
+ * @param {Object} [options.commonProperties={}] GeoJSON properties passed to ALL isolines


I would most likely change the name of the param to properties.

Optional comment... I do agree that this module has lots of different properties params/variables, it is a bit confusing if it's not defined as commonProperties

DenisCarriere · 2017-06-08T04:39:41Z

packages/turf-isolines/index.js

- * @returns {FeatureCollection<LineString>} isolines
+ * @param {Array<number>} breaks where to draw isolines
+ * @param {string} [zProperty='elevation'] the property name in `points` from which z-values will be pulled
+ * @param {Object} [options={}] options on output


If you decide to use options, then all "optional" params should be included ~~(zProperty in options)~~ (would break backwards compatibility)

Or just drop options all together and just add 2 extra more params without the Object, there's very rare amounts of TurfJS modules that use this type of syntax.

(would break backwards compatibility)

Well actually compatibility is already an issue, since the resolution parameter is not used/useful anymore (it is defined by the pointGrid input itself).
Maybe we keep this module update for the next major release?

there's very rare amounts of TurfJS modules that use this type of syntax

The idea here is more similar the property object that for example is passed to all the turf-helpers function when creating a new feature (e.g. point([0,3], properties)). Here we have multiple isolines that are being generated, and you can pass a single properties object to all the isolines and/or specific properties to specific isolines.
turf-isoband currently follows this same syntax; we can improve the naming/description in there or change to two separate parameters in both modules.

DenisCarriere · 2017-06-08T04:41:28Z

packages/turf-isolines/index.js

+ * @param {Array<number>} breaks where to draw isolines
+ * @param {string} [zProperty='elevation'] the property name in `points` from which z-values will be pulled
+ * @param {Object} [options={}] options on output
+ * @param {Array<Object>} [options.isolineProperties=[]] GeoJSON properties passed, in order, to the correspondent


I don't quite understand this param...? We might want to improve the description a bit

If you look at the bigMatrix test it is how you can pass for example different colors to the isolines. The first property of the isolineProperty will be assigned to the first isoline (defined by the first breaks value), the second isolineProperty to the second isoline and so on.
Don't know how to better describe it in a concise way though...

DenisCarriere · 2017-06-08T04:44:30Z

packages/turf-isolines/index.js

-        }
+module.exports = function (points, breaks, zProperty, options) {
+    // Input validation
+    var isObject = function (input) {


No need to nest this function inside another function.

Optional comment...

DenisCarriere

👍 Looks good! I posted a few comments, but they are totally optional

stebogit · 2017-06-08T04:58:39Z

Thanks @DenisCarriere, always good to improve the code 😃
I'll clean up as soon as I have few minutes

stebogit · 2017-06-09T06:51:29Z

@DenisCarriere I implemented your suggestions, please take a look to index.d.ts cause I might have done it wrong.
I named the last optional parameter properties, which defines properties.perIsoline and properties.toAllIsolines, hopefully now is more clear.
Let me know what you think.

DenisCarriere · 2017-06-11T15:14:20Z

packages/turf-isolines/index.d.ts

-type Points = GeoJSON.FeatureCollection<GeoJSON.Point>;
-type LineStrings = GeoJSON.FeatureCollection<GeoJSON.LineString>;
+interface Properties {
+    perIsoline: Array<Object>,


👍 Looks good, only thing is since perIsolines & toAllIsolines are optional, you will need to add :? to make them optional.

interface Properties { perIsoline?: Array<Object>, toAllIsolines?: Object }

However, it might be best to not have those properties tags as an Object and just by their own param.

declare function isolines( points: Points, breaks: number[], zProperty?:string, propertiesPerIsoline?: Object[], propertiesToAllIsolines?: Object): MultiLineStrings;

javascript code

isolines(points, breaks, 'temperature', [{foo: 'bar'}], properties)

Also propertiesPerIsoline, seems a bit more complex to use vs. the propertiesToAllIsolines. We might want to reverse them.

const properties = {foo: 'bar'} isolines(points, breaks, 'temperature', properties) isolines(points, breaks, 'temperature', properties, [{name: 'break1'}, {name: 'break2'])

@stebogit

- Divide properties={} to two params - Simplify Catch error logic (index.js) - Update Typescript definition - Add types.ts to test Typescript definition - Add single task to benchmark - Update Yarn.lock - Simplify bench & test logic - Save fixtures out to GeoJSON CC: @stebogit

DenisCarriere · 2017-06-11T16:39:54Z

👍 @stebogit You've outdone yourself this time, this module looks great! Can't wait to try this out on some big datasets.

DenisCarriere · 2017-06-11T17:11:48Z

Updates

Split properties={} to two params propertiesToAllIsolines & propertiesPerIsoline
Simplify Catch error logic (index.js)
Update Typescript definition
Add types.ts to test Typescript definition
Add single task to benchmark
Update Yarn.lock
Simplify bench & test logic
Save fixtures out to GeoJSON
perIsoline overlaps properties from toAllIsolines
Update README

DenisCarriere · 2017-06-11T17:24:59Z

👍 Going to merge, feel free to send commits on the master branch for any more little tweaks.

@stebogit

* added fiji + resolute-bay tests to translate * added fiji + resolute-bay test to rotate * Add new modules to Turf core * v4.4.0 * Fix tests for blank dependencies * Add inside tests * New @turf/isolines based on MarchingSquares.js (#781) * replaced conrec with marchingsquare * fixed script and tests * updated bench * updated readme * updated package.json * updated index.d.ts * introduced suggested changes * added zProperty to index.d.ts * minor doc change * Updates to Isolines - Divide properties={} to two params - Simplify Catch error logic (index.js) - Update Typescript definition - Add types.ts to test Typescript definition - Add single task to benchmark - Update Yarn.lock - Simplify bench & test logic - Save fixtures out to GeoJSON CC: @stebogit * perIsoline overlaps properties from toAllIsolines * Update Readme * Buffer (#786) Buffer - Drop circle buffer operation * removed turf-isolines old test files * copied files from repo * refactored index and test * updated README * updated index.d.ts and bench * added yarn.lock * removed Polygon as accepted input; added array test; * added masking feature to turf-grid * added throws tests * updated yarn.lock * added contributor * Resolves #785 * Add test fixture * Update complex unkink-polygon * Update name to boolean-clockwise * Declare input as line instead of Feature

stebogit added 6 commits June 7, 2017 01:07

replaced conrec with marchingsquare

4877842

fixed script and tests

d5fa41e

updated bench

6456b87

updated readme

9c53e76

updated package.json

1efed7b

updated index.d.ts

f67dbcd

stebogit added the enhancement label Jun 7, 2017

stebogit self-assigned this Jun 7, 2017

stebogit requested a review from DenisCarriere June 7, 2017 16:08

stebogit force-pushed the isolines branch from cced2cf to f67dbcd Compare June 8, 2017 03:33

DenisCarriere reviewed Jun 8, 2017

View reviewed changes

DenisCarriere approved these changes Jun 8, 2017

View reviewed changes

stebogit added 2 commits June 8, 2017 23:35

introduced suggested changes

3b4a180

added zProperty to index.d.ts

039beee

minor doc change

2821fc4

DenisCarriere reviewed Jun 11, 2017

View reviewed changes

perIsoline overlaps properties from toAllIsolines

d8369f3

Update Readme

8931541

DenisCarriere merged commit 321d0df into master Jun 11, 2017

DenisCarriere deleted the isolines branch June 11, 2017 17:25

DenisCarriere added this to the 4.5.0 milestone Jun 12, 2017

DenisCarriere mentioned this pull request Jun 30, 2017

New minor release! Turf 4.5.2 🎉 #826

Closed

mvl22 mentioned this pull request Nov 15, 2018

Dependency @turf/isolines has changed its API so version needs to be fixed mapbox/osrm-isochrone#31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New @turf/isolines based on MarchingSquares.js #781

New @turf/isolines based on MarchingSquares.js #781

stebogit commented Jun 7, 2017

DenisCarriere commented Jun 7, 2017 •

edited

Loading

stebogit commented Jun 7, 2017

DenisCarriere commented Jun 7, 2017 •

edited

Loading

stebogit commented Jun 7, 2017 •

edited

Loading

DenisCarriere commented Jun 7, 2017

DenisCarriere commented Jun 7, 2017

stebogit commented Jun 7, 2017

DenisCarriere commented Jun 7, 2017

stebogit commented Jun 8, 2017

DenisCarriere Jun 8, 2017

DenisCarriere Jun 8, 2017 •

edited

Loading

DenisCarriere Jun 8, 2017 •

edited

Loading

DenisCarriere Jun 8, 2017

stebogit Jun 9, 2017 •

edited

Loading

DenisCarriere Jun 8, 2017

stebogit Jun 9, 2017

DenisCarriere Jun 8, 2017 •

edited

Loading

DenisCarriere left a comment

stebogit commented Jun 8, 2017

stebogit commented Jun 9, 2017

DenisCarriere Jun 11, 2017

DenisCarriere Jun 11, 2017

DenisCarriere commented Jun 11, 2017

DenisCarriere commented Jun 11, 2017 •

edited

Loading

DenisCarriere commented Jun 11, 2017

New @turf/isolines based on MarchingSquares.js #781

New @turf/isolines based on MarchingSquares.js #781

Conversation

stebogit commented Jun 7, 2017

DenisCarriere commented Jun 7, 2017 • edited Loading

stebogit commented Jun 7, 2017

DenisCarriere commented Jun 7, 2017 • edited Loading

stebogit commented Jun 7, 2017 • edited Loading

DenisCarriere commented Jun 7, 2017

DenisCarriere commented Jun 7, 2017

stebogit commented Jun 7, 2017

DenisCarriere commented Jun 7, 2017

stebogit commented Jun 8, 2017

DenisCarriere Jun 8, 2017

Choose a reason for hiding this comment

DenisCarriere Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

DenisCarriere Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

DenisCarriere Jun 8, 2017

Choose a reason for hiding this comment

stebogit Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

DenisCarriere Jun 8, 2017

Choose a reason for hiding this comment

stebogit Jun 9, 2017

Choose a reason for hiding this comment

DenisCarriere Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

DenisCarriere left a comment

Choose a reason for hiding this comment

stebogit commented Jun 8, 2017

stebogit commented Jun 9, 2017

DenisCarriere Jun 11, 2017

Choose a reason for hiding this comment

DenisCarriere Jun 11, 2017

Choose a reason for hiding this comment

DenisCarriere commented Jun 11, 2017

DenisCarriere commented Jun 11, 2017 • edited Loading

DenisCarriere commented Jun 11, 2017

DenisCarriere commented Jun 7, 2017 •

edited

Loading

DenisCarriere commented Jun 7, 2017 •

edited

Loading

stebogit commented Jun 7, 2017 •

edited

Loading

DenisCarriere Jun 8, 2017 •

edited

Loading

DenisCarriere Jun 8, 2017 •

edited

Loading

stebogit Jun 9, 2017 •

edited

Loading

DenisCarriere Jun 8, 2017 •

edited

Loading

DenisCarriere commented Jun 11, 2017 •

edited

Loading