Turf Transform #752

stebogit · 2017-05-22T08:03:44Z

Implemented first module @turf/transform-translate
Note: I used @turf/rhumb-destination to create the translation vector, which defines the motion; needs to be checked if the length of the resulting translation matches the provided distance.

Please feel free to comment, suggest, correct, add tests.

(Ref: #747)

DenisCarriere · 2017-05-23T05:52:12Z

👍 awesome stuff @stebogit I'm going to find the time this week to have a look at this in more details.

Keep it up!

stebogit · 2017-05-23T15:40:40Z

@dpmcmlxxvi @morganherlocker @DenisCarriere I implemented the first module transform-translate using a matrix multiplication as I thought it would have been easy to use the same pattern with all the other transformations.
Although this approach works fine (setting the distance issue aside for now), I realized that for example in turf-rotation it would mean first translate the feature to the origin (i.e. point([0,0])), then perform the rotation, then translate it back to the centroid. This I believe might result slow(er) and introduce approximation errors.
The alternative would be using turf-rhumb-destination on each point for the translation, and I guess turf-arc-line on each point for the rotation.
However, having matrix operations as a base "format" for the transformation I do believe might be helpful pattern to create other transformations (flip, mirror, projections, etc.). It also helps to handle the third coordinate.

Thoughts? Suggestions?

stebogit · 2017-05-23T15:51:24Z

Also, I worked based on the assumption that the transformations were meant to be on the plain, forgetting about, or better ignoring on purpose, the earth's curvature. This would make the transformations valid only in relatively "small" (compared to earth radius) areas.

DenisCarriere · 2017-05-23T15:54:04Z

Using the rhumb modules is perfectly fine! I even prefer this type of transformation since it plays "nicer" with Web Applications using Web Mercator projection (Google maps/Bing/Mapbox/ESRI).

dpmcmlxxvi · 2017-05-23T17:24:28Z

I agree that the rhumb approach might be more appropriate. It would be nice to have a common implementation (like transformation matrices) that could be re-used by multiple transforms but I think most users would expect a transformation in a geospatial library to perform geospatial friendly transformations and take the earth's curvature into account.

FWIW, there's nothing wrong with having a more efficient implementation that makes certain assumptions (e.g., local plane approximation) but you may want to implement that as an option if users complain about performance issues and not preemptively optimize.

stebogit · 2017-05-24T01:15:00Z

👍 I'll rewrite it then

stebogit · 2017-05-24T08:16:02Z

I implemented turf-rotate on the x,y plane; for now I did not include a rotation by the other axes.
Do we want to add that capability?

DenisCarriere · 2017-05-24T14:02:57Z

packages/turf-transform-rotate/index.d.ts

+/**
+ * http://turfjs.org/docs/#transform-rotate
+ */
+declare function rotate(geojson: Feature<any> | GeometryObject,


🥇 These Typescript definitions are not obvious, the goal with this would be to have the Input be equal to the Ouptut.

Which would equal to:

Geometry<Point> => Feature<Point>

Feature<Polygon> => Feature<Polygon>

Right now the current state mutates the geometry type to any:

Geometry<Point> => Feature<any>

Feature<Polygon> => Feature<any>

rotate

type GeometryObject = GeoJSON.GeometryObject; type Feature<Geom extends GeometryObject> = GeoJSON.Feature<Geom>; type Point = Feature<GeoJSON.Point> | GeoJSON.Point | number[]; /** * http://turfjs.org/docs/#transform-rotate */ declare function rotate<Geom extends GeometryObject>( geojson: Feature<Geom> | Geom, angle: number, pivot?: Point): Feature<Geom>;

Example from translate

type GeometryObject = GeoJSON.GeometryObject; type Feature<Geom extends GeometryObject> = GeoJSON.Feature<Geom>; /** * http://turfjs.org/docs/#transform-translate */ declare function translate<Geom extends GeometryObject>( geojson: Feature<Geom> | Geom, distance: number, direction: number, units?: string): Feature<Geom>;

Note: I'll push a commit to update those definitions.

DenisCarriere · 2017-05-24T14:08:20Z

packages/turf-transform-rotate/bench.js

+ */
+const suite = new Benchmark.Suite('turf-transform-rotate');
+for (const {name, geojson} of fixtures) {
+    let {angle, pivot} = geojson.properties || {};


let is used if the variable will be reassigned, in this scenario const should be used since it doesn't change after it's been declared.
This code will still work, but some aggressive linters have started to catch this as an linting error.

DenisCarriere · 2017-05-24T14:15:19Z

packages/turf-transform-translate/index.js

+ * @param {number} distance length of the motion; negative values determine motion in opposite direction
+ * @param {number} direction of the motion; angle from North between -180 and 180 decimal degrees, positive clockwise
+ * @param {string} [units=kilometers] in which `distance` will be express; miles, kilometers, degrees, or radians
+ *     (optional, default `kilometers`)


(optional, default "kilometers") is not required since it's already defined as optional using the square brackets.

/** * @param {string} [units=kilometers]

DenisCarriere · 2017-05-24T14:19:01Z

packages/turf-transform-translate/package.json

+  "dependencies": {
+    "@turf/helpers": "^4.3.0",
+    "@turf/invariant": "^4.3.0",
+    "@turf/meta": "^4.3.0",


Since @turf/meta does not exist in the index.js there's a new test in package/turf/test.js that will catch this as an error.

DenisCarriere · 2017-05-24T14:21:20Z

packages/turf-transform-translate/test.js

+const fs = require('fs');
+const load = require('load-json-file');
+const write = require('write-json-file');
+const helpers = require('@turf/helpers');


Since the tests are now written in ES6 (NodeJS 6+) you can use the following when importing multiple methods from a module:

const {point, featureCollection} = require('@turf/helpers');

DenisCarriere · 2017-05-24T14:24:38Z

packages/turf-transform-translate/test.js

+
+        const translated = translate(geojson, distance, direction, units, zTranslation);
+
+        // style result


Wrap these styled results as their own dedicated functions (ex: colorize(geojson)).

That way the tests can be more readable and the colorize method can be reused if need be.

function colorize(geojson) { // style result if (geojson.geometry.type === 'Point' || geojson.geometry.type === 'MultiPoint') { geojson.properties['marker-color'] = '#F00'; geojson.properties['marker-symbol'] = 'star'; } else { geojson.properties['stroke'] = '#F00'; geojson.properties['stroke-width'] = 4; } return geojson; }

DenisCarriere · 2017-05-24T14:30:00Z

packages/turf-transform-rotate/README.md

+var rotatedPoly = turf.rotate(poly, 85);
+
+//addToMap
+var addToMap = [rotatedPoly];


Might want to include "before" & "after", simply seeing 1 polygon won't show you much (might want to change the colour of the fill).

DenisCarriere · 2017-05-24T14:31:59Z

packages/turf-transform-rotate/index.d.ts

+import {Point} from '@turf/helpers'
+
+export type GeometryObject = GeoJSON.GeometryObject;
+export type Feature<Geom extends GeometryObject> = GeoJSON.Feature<Geom>;


Since this modules are single export methods, you can only have 1 dedicated export, can't have multiple exports.

index.d.ts(15,1): error TS2309: An export assignment cannot be used in a module with other exported elements.

DenisCarriere · 2017-05-24T14:33:46Z

packages/turf-transform-rotate/index.js

+ * @param {number} [yRotation=0] angle of rotation, respect to the horizontal (x,y) plain, about the y axis;
+ * @returns {Feature<any>} the rotated GeoJSON feature
+ * @example
+ * var poly = turf.polygon([[0,29],[3.5,29],[2.5,32],[0,29]]);


invalid polygon 🤦‍♂️ , needs an extra []

We should add these tests directly in the main Turf tests package/turf/test.js.

A test that actually tests if all the JSDocs are being tested, it's currently doing this in turf-www.

@rowanwins

- Typescript definition - Drop extra dependencies - Update readme - Minor refactoring to tests - Added typescript tests

DenisCarriere · 2017-05-24T14:58:02Z

packages/turf-transform-rotate/test.js

+            rotated.properties["stroke"] = "#F00";
+            rotated.properties["stroke-width"] = 4;
+        }
+        if (pivot) {


This type of logic makes the tests confusing.

Simplified this as a single function.

function makePivot(pivot, geojson) { if (!pivot) return centroid(geojson, {'marker-symbol': 'circle'}); return point(getCoord(pivot), {'marker-symbol': 'circle'}); }

DenisCarriere · 2017-05-24T15:19:09Z

packages/turf-transform-rotate/test/in/polygon.geojson

+  "type": "Feature",
+  "properties": {
+    "angle": 200,
+    "pivot": {


Not too sure if pivot should be nested as a GeoJSON point, could simply be an Array [2.48291015625, 27.819644755099446]

Noticed you applied this change to other fixtures (looks like this was the only one) 👍

stebogit · 2017-05-25T07:14:02Z

packages/turf-transform-rotate/test.js

@@ -45,8 +45,6 @@ test('rotate -- throws', t => {
    t.throws(() => rotate(featureCollection([line]), 100), /FeatureCollection is not supported/, 'featureCollection');
    t.throws(() => rotate(geometryCollection([line.geometry]), 100), /GeometryCollection is not supported/, 'geometryCollection');


@DenisCarriere this currently fails because geojson.type == 'Feature' so it won't trigger the Error; was it working before?

stebogit · 2017-05-25T07:25:50Z

Man now the code looks so nice! Love it! 😃

@DenisCarriere in regard to this your comment I'm not sure I got what you mean.
Currently transform-translate uses a matrix for the transformation, which I think is neither a rhumb nor a haversine motion/transformation, am I right?
I was thinking to rewrite it with turf-rhumb-destination and coordEach applying there the same clean logic just used for transform-rotate.
What do you think?

stebogit · 2017-05-25T07:52:09Z

Added a test case for the z coord.
I noticed that the extra coordinate value is actually correctly handled by the transformation, however turf-truncate in test.js removes it, so the test fails.

Is that an expected result of truncate?

DenisCarriere · 2017-05-25T13:23:03Z

@stebogit Turf truncate removes the z coordinate by default, however you can add coordinate 3 to include the z coordinate, maybe we should change the default value to 3 coordinates instead of 2 since GeoJSON allows 6 decimals & 3 coordinates.

@stebogit

- Reflect tests to support all GeoJSON - Update JSDocs to support all GeoJSON - Update typescript to All GeoJSON types - Add mutate param CC: @stebogit

@stebogit

- Add Tap for testing (easier to read results) CC: @stebogit

DenisCarriere · 2017-05-25T14:32:41Z

@stebogit Are you planning on refactoring @turf/transform-translate to coordEach()?

I can help refactor it after you've done your first pass at it.

coveralls · 2017-05-25T14:39:18Z

Changes Unknown when pulling 68553b2 on turf-transform into ** on master**.

stebogit · 2017-05-25T14:51:01Z

packages/turf-transform-rotate/index.d.ts

@@ -11,7 +11,8 @@ declare function rotate<Geom extends Geoms>(
    angle: number,
    pivot?: Point,
    xRotation?: number,
-    yRotation?: number): Geom;
+    yRotation?: number,


Are we keeping xRotation and yRotation or this is just a leftover?

stebogit · 2017-05-25T14:54:04Z

@DenisCarriere yes I am 😃 , but I won't be able to do it until tonight (PST).
Feel free to go on if you want and have time to do it. I'll still have turf-transform-scale to play with tonight 😄

DenisCarriere · 2017-05-25T15:08:32Z

👍 I'll let you tackle that one, lots of stuff is going on in that repo and it's going to go a lot faster if you do it since you built it, i'd get stomped if I get an error at the end. However @turf/transform-rotate is looking pretty slick, it's a good example to use as a starting point.

coveralls · 2017-05-25T15:10:15Z

Changes Unknown when pulling 6d98ed9 on turf-transform into ** on master**.

DenisCarriere · 2017-05-25T16:46:08Z

These coveralls notifications are cool, but also a bit annoying.

Going to remove this for now, but I'm looking forward to have coverage feedback in the PRs.

dpmcmlxxvi · 2017-05-26T00:46:49Z

packages/turf-transform-translate/index.js

+    // Clone geojson to avoid side effects
+    if (mutate === false || mutate === undefined) geojson = JSON.parse(JSON.stringify(geojson));
+
+    var motionVector = getCoords(rhumbDestination([0, 0], distance, direction, units));


@stebogit Not sure if this has been discussed but this motionVector based translation approach is not doing what you may be expecting it to do. The rhumb line should be calculated from the given point not from the origin and then translated. Won't give the same answer. You should be able to apply the same logic from the scale approach and just apply the rhumb transformation in the coordEach loop.

OK, just saw the comment where you were planning on doing this.

updated bench; updated README; restored geometry support tests; removed unused dependencies;

stebogit · 2017-05-26T07:13:20Z

packages/turf-transform-translate/test.js

+    t.assert(translate(geometryCollection([line.geometry]).geometry, 100, 50), 'geometryCollection support');
+    t.assert(translate(featureCollection([line]), 100, 50), 'featureCollection support');
+    t.assert(translate(line.geometry, 100, 50), 'geometry line support');
+    t.assert(translate(line.geometry, 100, 50), 'geometry pt support');
    // t.assert(translate(line.geometry.coordinates, 100, 50), 'pt coordinate support');


@DenisCarriere I believe this test, which fails, is actually wrong; we are passing here an array of coordinates, which is not a GeoJSON object

That's true, this should throw an error 👍

coveralls · 2017-05-26T07:17:52Z

Changes Unknown when pulling 23d18a1 on turf-transform into ** on master**.

coveralls · 2017-05-26T07:17:52Z

Changes Unknown when pulling 23d18a1 on turf-transform into ** on master**.

DenisCarriere · 2017-05-26T12:00:18Z

packages/turf-transform-translate/index.js

-        var translatedCoords = [].concat.apply([], resultVector); // flatten translated vector
-        return translatedCoords;
+    // Translate each coordinate
+    coordEach(geojson, function (pointCoords) {


Wow, what a big change! 👍 Looks too good to be true!

coveralls · 2017-05-26T12:20:09Z

Changes Unknown when pulling 3e64de8 on turf-transform into ** on master**.

coveralls · 2017-05-26T12:26:10Z

Changes Unknown when pulling 0cd7b6b on turf-transform into ** on master**.

coveralls · 2017-05-26T12:29:06Z

Changes Unknown when pulling 0cd7b6b on turf-transform into ** on master**.

stebogit · 2017-05-26T15:31:14Z

@DenisCarriere how come that the branch has been merged but it is shown as still separate in the graph?

Can I still work on the transform branch to implement transform-scale?
and, if yes, once I'm done should I create a new PR on this same branch?

DenisCarriere · 2017-05-27T15:43:03Z

@stebogit We've noticed that in the past, I believe the reason is because when I merged I chose "Squash and merge". I don't know if that's an intended feature/bug in GitHub.

It's best to start a new branch (like you've done).

stebogit added 4 commits May 20, 2017 03:39

first translate implementation

ddcb8cb

rifactored multiplyMatrices; modified tests;

9cb8f30

implemented multi features support; add various tests;

96e9709

updated bench.js

0211f3c

stebogit added the new-module label May 22, 2017

stebogit self-assigned this May 22, 2017

stebogit requested a review from DenisCarriere May 22, 2017 08:04

stebogit changed the title ~~Turf Transform (Ref: #747)~~ Turf Transform May 22, 2017

DenisCarriere added this to the 4.4.0 milestone May 23, 2017

stebogit added 2 commits May 24, 2017 01:01

adds transform-rotate module

398166e

added throws tests

a74df7a

DenisCarriere reviewed May 24, 2017

View reviewed changes

Update @turf/transform-translate

e9f3fda

- Typescript definition - Drop extra dependencies - Update readme - Minor refactoring to tests - Added typescript tests

DenisCarriere reviewed May 24, 2017

View reviewed changes

stebogit commented May 25, 2017

View reviewed changes

add test for z-coordinate support

922e6e1

DenisCarriere added 2 commits May 25, 2017 10:07

Update param, docs & tests @turf/transform-rotate

bfb4d35

- Reflect tests to support all GeoJSON - Update JSDocs to support all GeoJSON - Update typescript to All GeoJSON types - Add mutate param CC: @stebogit

Update JSDocs & tests transform translate

68553b2

- Add Tap for testing (easier to read results) CC: @stebogit

stebogit commented May 25, 2017

View reviewed changes

Drop meta from dependencies

6d98ed9

dpmcmlxxvi reviewed May 26, 2017

View reviewed changes

refactored transform with coordEach;

23d18a1

updated bench; updated README; restored geometry support tests; removed unused dependencies;

stebogit commented May 26, 2017

View reviewed changes

DenisCarriere reviewed May 26, 2017

View reviewed changes

DenisCarriere added 2 commits May 26, 2017 08:07

Add default zTranslation to 0

7e56671

Add does mutate tests

3e64de8

fix types tests

0cd7b6b

DenisCarriere merged commit 4c2af24 into master May 26, 2017


		const translated = translate(geojson, distance, direction, units, zTranslation);

		// style result

		@@ -45,8 +45,6 @@ test('rotate -- throws', t => {
		t.throws(() => rotate(featureCollection([line]), 100), /FeatureCollection is not supported/, 'featureCollection');
		t.throws(() => rotate(geometryCollection([line.geometry]), 100), /GeometryCollection is not supported/, 'geometryCollection');

Turf Transform #752

Turf Transform #752

Conversation

stebogit commented May 22, 2017 • edited

DenisCarriere commented May 23, 2017

stebogit commented May 23, 2017 • edited

stebogit commented May 23, 2017

DenisCarriere commented May 23, 2017

dpmcmlxxvi commented May 23, 2017 • edited

stebogit commented May 24, 2017

stebogit commented May 24, 2017

DenisCarriere May 24, 2017 • edited

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 May 24, 2017 • edited

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 May 25, 2017

stebogit commented May 25, 2017

DenisCarriere commented May 25, 2017

DenisCarriere commented May 25, 2017

coveralls commented May 25, 2017 • edited

stebogit May 25, 2017 • edited

Choose a reason for hiding this comment

stebogit commented May 25, 2017

DenisCarriere commented May 25, 2017

coveralls commented May 25, 2017

DenisCarriere commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stebogit May 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 26, 2017

coveralls commented May 26, 2017

Choose a reason for hiding this comment

coveralls commented May 26, 2017

coveralls commented May 26, 2017

coveralls commented May 26, 2017

stebogit commented May 26, 2017

DenisCarriere commented May 27, 2017

stebogit commented May 22, 2017 •

edited

stebogit commented May 23, 2017 •

edited

dpmcmlxxvi commented May 23, 2017 •

edited

DenisCarriere May 24, 2017 •

edited

DenisCarriere May 24, 2017 •

edited

coveralls commented May 25, 2017 •

edited

stebogit May 25, 2017 •

edited

stebogit May 26, 2017 •

edited