New @turf-isobands module with MarchingSquares #619

stebogit · 2017-03-15T14:04:26Z

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 occured.
Run npm run lint to ensure code style at the turf module level. --> not sure how/if it works

stebogit · 2017-03-15T14:14:11Z

@DenisCarriere
I tried to run eslint but I got so many errors (like 18000)... Is it possible that they are not only caused by my code? I noticed that it's asked to use AirBnB style guide, which has recently deprecated ES5, while turf uses ES5 only. Isn't that an issue?

I don't know how travis works, could you please give me some hint on how to set/fix it?

Thanks

DenisCarriere · 2017-03-15T14:33:40Z

@stebogit 👍 Thanks attempting this, the linting is done at the TurfJS root level.

Once you are in the root folder:

$ npm install
$ npm run lint

So far the isobands turf linting related errors are:

You can also see this in the build logs https://travis-ci.org/Turfjs/turf/builds/211360094

/home/travis/build/Turfjs/turf/packages/turf-isobands/index.js
   12:1   error  Expected JSDoc for 'pointGrid' but found 'grid'                                                valid-jsdoc
   38:1   error  Trailing spaces not allowed                                                                    no-trailing-spaces
   40:1   error  Trailing spaces not allowed                                                                    no-trailing-spaces

stebogit · 2017-03-15T14:42:40Z

thanks!
I'll fix the issues later this evening.
I see also marchingsquare.js has a bunch of styling issues, is it acceptable being that an external (minified) library?

DenisCarriere · 2017-03-15T14:34:52Z

packages/turf-isobands/.travis.yml

+  - "4"
+before_install:
+  - npm install -g npm@~3.8.9
+


This file is not needed since TurfJS has .travis.yml at the root level

DenisCarriere · 2017-03-15T14:35:34Z

packages/turf-isobands/.npmignore

@@ -0,0 +1,2 @@
+test
+coverage


Not needed since this is covered in the package.json using "files": ["index.js", "index.d.ts"]

What exactly is not needed? the .npmignore file?

The entire .npmignore file is not required, I've changed every module in Turf so all of them have the same syntax & file structure.

Use any turf module as your starting point to propose a new module (except the index.js).
https://github.com/Turfjs/turf/tree/master/packages/turf-along

DenisCarriere · 2017-03-15T14:36:21Z

packages/turf-isobands/LICENSE

@@ -0,0 +1,21 @@
+The MIT License (MIT)
+
+Copyright (c) 2016 Stefano Borghi


We've been using TurfJS as a generic name for all licenses (also 2017)

DenisCarriere · 2017-03-15T14:37:43Z

packages/turf-isobands/bench.js

+  .on('complete', function () {
+
+  })
+  .run();


Feel free to write in ES6 👍 bench.js & test.js is your only chance to write arrow functions :)

DenisCarriere · 2017-03-15T14:38:38Z

packages/turf-isobands/index.d.ts

+ * http://turfjs.org/docs/#isolines
+ */
+declare function isobands(points: Points, z: string, resolution: number, breaks: Array<number>): LineStrings;
+declare namespace isolines { }


namespace must be the same name as the function name.

This breaks the typescript definition.

DenisCarriere · 2017-03-15T14:42:30Z

packages/turf-isobands/index.js

+ * var breaks = [0, 5, 8.5];
+ * var isobands = turf.isobands(pointGrid, 'z', breaks);
+ * //=isobands
+ ******************************************************************/


Same as above **/

DenisCarriere · 2017-03-15T14:43:40Z

packages/turf-isobands/index.js

+     divide points in pointGrid by latitude, creating a 2-dimensional data grid
+     ####################################*/
+    var pointsByLatitude = {};
+    for (var j = 0; j < points.length; j++) {


Try using one of the @turf/meta functions, coordEach() or featureEach()

DenisCarriere · 2017-03-15T14:45:31Z

packages/turf-isobands/package.json

+  "name": "turf-isobands",
+  "version": "2.0.0",
+  "description": "turf isobands module",
+  "main": "index.js",


Include "types":"index.d.ts" (next to main instead of at the bottom) & "files": ["index.js", "index.d.ts"]

DenisCarriere · 2017-03-15T14:45:44Z

packages/turf-isobands/package.json

+  "description": "turf isobands module",
+  "main": "index.js",
+  "scripts": {
+    "test": "tape test.js"


add benchmark to scripts

is "bench": "node bench.js" ok?

DenisCarriere · 2017-03-15T14:49:35Z

packages/turf-isobands/test.js

+var isobands = require('./');
+
+test('isobands', function(t){
+  var points = JSON.parse(fs.readFileSync(__dirname + '/geojson/Points.geojson'));


Try to make your tests have dynamic data loading instead of hard coded fixtures in your tests.
That way someone can expand easily the test cases by simply adding a new .geojson to the test/in.

@turf/union has a good example of that:
https://github.com/Turfjs/turf/blob/master/packages/turf-union/test.js

DenisCarriere · 2017-03-15T14:54:22Z

@stebogit As for the MarchingSquare.js file, I would not include it as a minified file, does it exists on npm?

stebogit · 2017-03-16T02:21:56Z

@DenisCarriere
no, MarchingSquare.js does not have an npm module (yet @RaumZeit ?). Is there a way to exclude it when linting?
Also, the name of the MS function Isobands is uppercase, which eslint doesn't like...

DenisCarriere · 2017-03-16T17:17:52Z

Quick eslint trick is you can include this around your code to ignore "valid" lines that cause errors.

/*eslint-disable*/
var isobands = MarchingSquaresJS.IsoBands(gridData, lowerBand, upperBand - lowerBand);
/*eslint-enable*/

DenisCarriere · 2017-03-16T17:29:14Z

@stebogit Glancing at the repo, I'm thinking we should drop (make optional) the @param {string} z in favor of the geometry.coordinates (x, y, z).

The GeoJSON specs only allows (longitude, latitude, elevation/altitude) to be a valid geometry.
http://geojson.org/geojson-spec.html#positions

Proposed valid input

var points = {
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "type": "Point",
        "coordinates": [
          7,
          42,
          300
        ]
      }
    },
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "type": "Point",
        "coordinates": [
          10,
          45,
          230
        ]
      }
    },
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "type": "Point",
        "coordinates": [
          5,
          45,
          215
        ]
      }
    }
  ]
}
var results = isobands(points, [200, 250, 300])

stebogit · 2017-03-16T17:37:36Z

packages/turf-isobands/test.js

-        t.deepEqual(isobanded, load.sync(directories.out + 'isobands' + idx), input.name);
+        if (process.env.REGEN) write.sync(directories.out + filename, results);
+        t.equal(results.features[0].geometry.type, 'MultiPolygon', name + ' geometry=MultiPolygon');
+        t.deepEqual(results, load.sync(directories.out + filename), name);


this way the code looks definitely nicer, but the content of the output file, points1.geojson, is not a collection of points, but actually of multipolygons or isobands.
don't you think it might be a little confusing?

The test/in & test/out should be the same filename. That way it's clear what goes in and what goes out.

You can even call points.geojson italy.geojson and then the output would be test/out/italy.geojson

This becomes more evident when you start adding more and more files in the test/in, for example @turf/union has many files.

https://github.com/Turfjs/turf/tree/master/packages/turf-union/test/in

then I'd call them input1.geojson, input2.geojson and so on 😄
not a biggie though

stebogit · 2017-03-16T17:56:00Z

I get your point.
However the input are points, which might have multiple properties you want to chart (I'm not charting elevation for example, but estimated count of bugs 😄), so specifying the name might be useful.
If made optional the default might certainly be elevation or altitude

DenisCarriere · 2017-03-16T18:19:14Z

@stebogit 👍 Valid point, I think having it optional would be a good idea, or at least have that param at the end of the function.

function (pointGrid, breaks, z)

We could have z as default z="elevation" or even better read the z coordinate from [x,y,z].

We can add that logic at the beginning as part of the user input validation by reading the first point entry and figuring out if his FeatureCollection<Point> is valid or not (if he is using properties or z coordinates).

stebogit · 2017-03-16T18:32:44Z

@DenisCarriere great! 🎉
z at the end of the function makes it easy and effective.
Never thought about having an input validation, good point! 👍

DenisCarriere · 2017-03-16T20:19:41Z

@stebogit I will have a stab at it over the weekend, also there's a lot of "complicated" logic inside the index.js that can be replaced by @turf/meta methods (coordEach(), featureEach(), etc..).

stebogit · 2017-03-16T20:31:28Z

@DenisCarriere Thanks, I'm not familiar with the meta functions and I haven't worked on my contour project so it might take a while for me to correctly make the changes.
I believe you are more into turf/GeoJSON standards and details for input validation.
Let me know if/how I can help though

DenisCarriere · 2017-03-16T21:42:34Z

No worries, I have this habit of refactoring code until the most minimal amounts of lines are needed.

At least we have a "functional" prototype of isobands, the next steps would simply be making sure the code is running as fast as it possibly can and without any memory leaks.

DenisCarriere · 2017-03-16T21:57:07Z

@stebogit another idea for the breaks param. We could have both of the following as valid inputs:

Array<number> (explicitly define each break)
number (define each break by a fix interval)

Scenario: User wants to create contour lines based on 25 meter intervals (elevation units must also be in meters or the desired unit which is the same as the breaks).

stebogit · 2017-03-17T05:54:28Z

@DenisCarriere splendid idea!

RaumZeit · 2017-03-19T12:37:18Z

MarchingSquaresJS is now available through npm as marchingsquares module. Note, however, that due to npm naming conventions I've reverted the public API function names to lower camel case since version 1.2.0 of MarchingSquaresJS, i.e. the IsoBands function is now named isoBands.

DenisCarriere · 2017-04-17T04:23:46Z

@stebogit Let me know if you are happy to merge this module, looks good to me 👍 .

Always room for improvements/fixes in future releases.

Also, feel free to add your matrix-to-grid & grid-to-matrix module once it's stable.

stebogit · 2017-04-17T05:08:53Z

@DenisCarriere personally I would remove the random input interpolation (the createPointGrid function is not actually working correctly), just requiring the input to be a pointGrid. I'll work on fixing the output offset and I want to create few more tests to make sure the output is actually right (here matrix-to-grid will be useful).

Also, feel free to add your matrix-to-grid & grid-to-matrix module once it's stable.

Do you mean in this module? My idea was to use them in @turf/interpolate (#24, still under construction...), which I thought would allow users to create/clean the input for isobands and isolines, leaving the input handling outside them. Or, we might include the validation with@turf/interpolate once completed.

DenisCarriere · 2017-04-17T05:20:09Z

Oh I thought that createGridData was grid-to-matrix, however it doesn't look like it's exactly the same thing.

👍 Yep, you can remove the random fixture, we can always add it later in a future minor release.

Looking forward to publishing this module! 🚀

@stebogit Have a last glance at this module and merge this PR whenever you feel comfortable with it.

stebogit · 2017-04-19T08:16:47Z

@DenisCarriere I created a few more tests for isobands and I can tell there are few things that are not quite right, the output is not reliable. MarchingSquare even seems to return some unexpected outputs sometimes, but I'm not sure yet.
I'll keep working on this, but unfortunately I believe it's not gonna be quick... sorry 😞

DenisCarriere · 2017-04-19T12:33:38Z

@stebogit No rush... (take all the time in the world 👍 ).

even seems to return some unexpected outputs sometimes

I agree, MarchingSquare is a bit foreign to me, you're the one with the most experience using this library.

Keep it up!

- set aside z-coordinate test; - fixed output dimensions issue; - added grid-to-matrix dependancy;

stebogit · 2017-04-24T08:45:15Z

Fixed output bug, next will add more tests with graphically easy-to-read grid/matrix input values, then will evaluate performances.

adds grid test;

added tests; updated benchmark; updated readme; introduced common and per isoband options;

stebogit · 2017-05-08T09:11:12Z

@DenisCarriere please take a look at the module, and feel free to add more tests.
I added a common and an individual properties object as options, useful to style the isobands.
Now that I think about it, for some reason I removed the use of z-coordinate 😅 , we probably want to restore it.
I think we could consider this done as a first working version.
Lots of room for improvements though, in particular in performance and output validation for more strict maps like Google.

DenisCarriere · 2017-05-09T02:40:26Z

Works for me! 👍 This will be released in the next minor release v4.3 (aiming to get that done ~~tonight~~ Soon 🤓 )

stebogit added 6 commits January 28, 2017 23:18

added isobands module

dcb39ca

updated to latest version of @Turf and MarchingSquaresJS

fa32920

integrated module.export to MS-isobands, added sandbox

def67a5

updated marchingsquare library

c42f537

removed sandbox

3341cd4

modified version and license

2093242

DenisCarriere requested changes Mar 15, 2017

View reviewed changes

stebogit and others added 4 commits March 15, 2017 19:33

made suggested corrections

7a40684

replaced minified marchingsquare

c96d13e

Refactoring turf-isobands

bdbb1a8

Merge branch 'master' into isobands

fb6cce8

stebogit commented Mar 16, 2017

View reviewed changes

Merge branch 'master' into isobands

6017a04

DenisCarriere approved these changes Mar 16, 2017

View reviewed changes

DenisCarriere added the new-module label Apr 17, 2017

DenisCarriere added this to the 4.2.0 milestone Apr 17, 2017

Merge remote-tracking branch 'upstream/master' into isobands

6650f73

stebogit added 3 commits April 22, 2017 00:57

Merge remote-tracking branch 'upstream/master' into isobands

a0b7c78

reordered methods

6998178

- removed random points tests;

0030a2b

- set aside z-coordinate test; - fixed output dimensions issue; - added grid-to-matrix dependancy;

stebogit added 3 commits April 24, 2017 22:43

Merge remote-tracking branch 'upstream/master' into isobands

0b63cd7

adds matrix outer frame;

e9fb0a8

adds grid test;

simplified code

b1127ec

DenisCarriere modified the milestones: 4.3.0, 4.2.0 Apr 30, 2017

stebogit added 3 commits May 1, 2017 00:41

Merge remote-tracking branch 'upstream/master' into isobands

42469e9

set matrix json as valid test input

0b36635

fixed output;

2ea0902

added tests; updated benchmark; updated readme; introduced common and per isoband options;

DenisCarriere modified the milestones: 4.3.0, 4.4.0 May 8, 2017

DenisCarriere merged commit 910d5f5 into Turfjs:master May 9, 2017

DenisCarriere modified the milestones: 4.3.0, 4.4.0 May 9, 2017

DenisCarriere mentioned this pull request May 12, 2017

New minor release! Turf 4.3.1 🎉 #734

Closed

		@@ -0,0 +1,21 @@
		The MIT License (MIT)

		Copyright (c) 2016 Stefano Borghi

New @turf-isobands module with MarchingSquares #619

New @turf-isobands module with MarchingSquares #619

Conversation

stebogit commented Mar 15, 2017 • edited Loading

stebogit commented Mar 15, 2017

DenisCarriere commented Mar 15, 2017 • edited Loading

stebogit commented Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenisCarriere commented Mar 15, 2017

stebogit commented Mar 16, 2017

DenisCarriere commented Mar 16, 2017

DenisCarriere commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stebogit commented Mar 16, 2017

DenisCarriere commented Mar 16, 2017

stebogit commented Mar 16, 2017

DenisCarriere commented Mar 16, 2017

stebogit commented Mar 16, 2017

DenisCarriere commented Mar 16, 2017

DenisCarriere commented Mar 16, 2017 • edited Loading

stebogit commented Mar 17, 2017

RaumZeit commented Mar 19, 2017

DenisCarriere commented Apr 17, 2017

stebogit commented Apr 17, 2017 • edited Loading

DenisCarriere commented Apr 17, 2017 • edited Loading

stebogit commented Apr 19, 2017

DenisCarriere commented Apr 19, 2017

stebogit commented Apr 24, 2017

stebogit commented May 8, 2017 • edited Loading

DenisCarriere commented May 9, 2017 • edited Loading

stebogit commented Mar 15, 2017 •

edited

Loading

DenisCarriere commented Mar 15, 2017 •

edited

Loading

stebogit commented Mar 15, 2017 •

edited

Loading

DenisCarriere commented Mar 16, 2017 •

edited

Loading

stebogit commented Apr 17, 2017 •

edited

Loading

DenisCarriere commented Apr 17, 2017 •

edited

Loading

stebogit commented May 8, 2017 •

edited

Loading

DenisCarriere commented May 9, 2017 •

edited

Loading