New module @turf/transform-scale #759

stebogit · 2017-05-27T08:30:17Z

@DenisCarriere @dpmcmlxxvi
The output is currently not correct: it is scaled successfully, but for some reason it does not reflect the factor value.
I implemented my proposal for the origin; I look forward to knowing your opinion.
Also, the scaling along the z-coordinate is not along a Rhumb line, but a simple multiplication by 'factor'. Is there a way, if it make sense at all, to perform such a scaling on z?

For the moment I'd keep it simple and consider only uniform scalings, i.e. scaling factor equal in all directions.

DenisCarriere · 2017-05-27T15:44:31Z

Keeping it simple is always the winner in my book! 👍 Looks good so far

DenisCarriere · 2017-05-27T15:53:13Z

Strange that the coverage doesn't compute correctly, it might be looking at the first coverage tests and then upload right away.

Tap will be used at the root level and not in the individual repos

DenisCarriere · 2017-05-27T16:41:51Z

packages/turf-transform-scale/index.js

+ * @name scale
+ * @param {GeoJSON} geojson object to be scaled
+ * @param {number} factor of scaling, positive values greater than 0
+ * @param {boolean} [fromCenter=true] point from which the scaling will occur; it will default to the most


👍 Love it!

We might need to re-word the JSDocs, sounds like the "default" is saying False but in fact it's True.

point from which the scaling will occur; it will default to the most south-west point of a bbox containing the geojson, if true the scaling will origin from the centroid of the geojson

DenisCarriere · 2017-05-27T16:44:09Z

packages/turf-transform-scale/index.js

+    if (!geojson) throw new Error('geojson is required');
+
+    if (geojson.type === 'Point')
+        return (mutate === false || mutate === undefined) ? geojson : JSON.parse(JSON.stringify(geojson));


?? Why the multiple mutate === undefined This should be on it's own and only defined once.

DenisCarriere · 2017-05-27T16:52:55Z

packages/turf-transform-scale/index.js

+    if (fromCenter === true || fromCenter === undefined) {
+        origin = centroid(geojson);
+    } else {
+        var box = bbox(geojson); // [ minX, minY, maxX, maxY ]


Calculating BBox can be slightly computational heavy, so if a bbox is already provided in the geojson (which is defined in the spec) we can use that vs. re-calculating it.

http://geojson.org/geojson-spec.html#bounding-boxes

var box = (geojson.bbox) ? geojson.bbox : bbox(geojson);

good to know!

DenisCarriere · 2017-05-27T16:56:58Z

packages/turf-transform-scale/index.js

+    if (mutate === false || mutate === undefined) geojson = JSON.parse(JSON.stringify(geojson));
+
+    // Scale each coordinate
+    coordEach(geojson, function (pointCoords) {


Going to be using coord as variable name since the singular term implies it's a point:

coord === [x, y]

coords === [[x1, y1], [x2, y2]]

DenisCarriere · 2017-05-27T17:09:02Z

@stebogit Got an idea for defining the point of origin.

What if allow a param called fromCorner with 5 options (ne, nw, se, sw, center):

var west = bbox[0];
var south = bbox[1];
var east = bbox[2];
var north = bbox[3];

ne/northeast/ topright => [east, north]
nw/northwest/ topleft => [west, north]
se/southeast/ bottomright => [east, south]
sw/southwest/ bottomleft => [west, south]
center/undefined => center

This would allow an application to scale any GeoJSON in any 5 direction with relative ease.

stebogit · 2017-05-27T17:15:44Z

@DenisCarriere super! 👍

DenisCarriere · 2017-05-27T17:17:41Z

@stebogit Cool! I'll add that, shouldn't be too long.

stebogit · 2017-05-27T17:45:40Z

packages/turf-transform-scale/README.md

+
+# scale
+
+Moves any geojson Feature or Geometry of a specified distance along a Rhumb Line


@DenisCarriere probably you can find a better description for the module, explaining the Rhumb "motion" of the point; this however is the translate description

dpmcmlxxvi · 2017-05-27T17:57:20Z

packages/turf-transform-scale/index.js

+    if (!factor) throw new Error('factor is required');
+
+    // Default params
+    var isMutate = mutate === false || mutate === undefined;


Is this logic correct? If mutate is false then isMutate is true? Also, if mutate is not defined then isMutate is true. The logic below says if isMutate is true then you clone the data. I think either the logic or the variable names are backwards. You pick.

I'd say just:
if (mutate === true) geojson = JSON.parse(JSON.stringify(geojson));
dropping isMutate

So if mutate is true then you clone the data? Seems backwards.

Arrrgh! You're right! Dumb me...
Then
if (!mutate === true) geojson = JSON.parse(JSON.stringify(geojson));
Still no need for isMutate
Right?

That should work. Though there should be enough tests to ensure that it works and the data is not cloning when it should be and vice versa.

However, I usually prefer logical conditions to be explicit and not implicit which require the reader to sit there and figure out what is going on and have to remember javascript falsey rules. Just my preference. But if it works it works.

stebogit · 2017-05-27T17:57:32Z

packages/turf-transform-scale/index.js


    // Shortcut no-scaling
-    if (factor === 1)
-        return (mutate === false || mutate === undefined) ? geojson : JSON.parse(JSON.stringify(geojson));
+    if (factor === 1) return geojson;


@DenisCarriere what about:
if (factor === 1 || geojson.geometry.type === 'Point' || geojson.type === 'Point') return geojson;
Scaling a Point is pointless 😄

this would skip only Point, not MultiPoint. Wouldn't it?

Yep MultiPoint won't be skipped. I'll add that in my next commit, wait out

dpmcmlxxvi · 2017-05-27T18:06:48Z

@stebogit Scaling z by a multiplication factor is fine. Rhumb lines are just along the earth's surface.

dpmcmlxxvi · 2017-05-27T18:11:56Z

@stebogit You might consider allowing the fromCorner to be either the five strings mentioned by @DenisCarriere or a Point. Plus the name fromCorner might be a little misleading since, for example, the center is not a corner. Maybe just origin.

DenisCarriere · 2017-05-27T18:13:10Z

packages/turf-transform-scale/index.js

+ *
+ * @name scale
+ * @param {GeoJSON} geojson object to be scaled
+ * @param {number} factor of scaling, positive values greater than 0


@stebogit Couldn't it also be negative scaling?

@DenisCarriere how do you scale negative?
0 - 1 => shrink
1 - Infinity => enlarge
< 0 ?
0 => a Point ? Does this make sense?

stebogit · 2017-05-27T18:18:45Z

@dpmcmlxxvi 👍

Maybe just origin.

Why not originPosition then? Clear, unmistakable.

stebogit · 2017-05-27T18:37:54Z

allowing the fromCorner to be [...] a Point

@dpmcmlxxvi @DenisCarriere could the origin be anywhere?
i.e. do we want to allow this:

I personally find it weird, not obvious (it looks more like a scale + translate), but if you think otherwise let's origin be a user-defined Point!

dpmcmlxxvi · 2017-05-27T18:52:36Z

I think lots of scaling operations would be performed from origin points that are not related to the features. For example, fixing sensor mis-calibrations, mis-registrations, etc.

@stebogit

CC: @stebogit

stebogit · 2017-05-27T19:07:35Z

packages/turf-transform-scale/index.js

-        var box = (geojson.bbox) ? geojson.bbox : bbox(geojson); // [ west, south, east, north ]
-        origin = point([box[0], box[1]]);
-    }
+    if (factor === 1 || isPoint) return geojson;

    // Clone geojson to avoid side effects
    if (isMutate) geojson = JSON.parse(JSON.stringify(geojson));


@DenisCarriere I'd move this before any return, since the mutate behaviour should be respected even if there is no actual scaling. That's why my original multiple mutate === undefined

stebogit · 2017-05-27T19:21:27Z

packages/turf-transform-scale/index.js

+ * @param {string} [fromCorner] sw/se/nw/ne/center
+ * @returns {Feature<Point>} Point origin
+ */
+function defineOrigin(geojson, fromCorner) {


Great job! 👍

added tests with origin as point;

stebogit · 2017-05-28T09:23:42Z

@DenisCarriere @dpmcmlxxvi I fixed a couple of errors in the calculation and added some more tests. Now I believe the output is correct.
However I believe centroid is unexpected as default central origin, I'd rather use center. What do you think?

Example MultiPolygon:

DenisCarriere · 2017-05-28T15:27:36Z

@stebogit @dpmcmlxxvi ~~Shouldn't it be best for Multi(Feature) to~~ have the origin point centered on each individual shape when doing the scale operations.

~~We could use FlattenEach for that (shouldn't be too much effort, very similar to coordEach syntax).~~

Edit: Nevermind, that would most likely be the case for a FeatureCollection and not a MultiFeature.

DenisCarriere · 2017-05-28T16:43:39Z

Handling FeatureCollection

@stebogit FeatureCollection should be handled as an exception, if origin is undefined or string we should apply the scale operation on each feature individually. If origin is provided as a Point then the entire FeatureCollection will be scaled using that Point.

Note: GeometryCollection should NOT be handled like a FeatureCollection since it is considered a single Feature.

Proposed (Multiple centroids per feature)

Before (Single centroid origin)

As per #759 (comment)

DenisCarriere · 2017-05-28T16:58:01Z

However I believe centroid is unexpected as default central origin, I'd rather use center. What do you think?

@stebogit Yep agreed, center might be a better "default" value.

dpmcmlxxvi · 2017-05-28T17:01:56Z

@stebogit It looks good to me. @DenisCarriere As for scaling each feature separately in a FeatureCollection, it's an interesting approach but I'm wondering what you expect the use case to be? What geospatial application would someone need to scale each feature separately from, say, their own centroid? I would think scaling each separately would be the exception and by default treat the collection as the others.

DenisCarriere · 2017-05-28T17:08:04Z

Actually centroid looks like it would be the better option vs. center.

Centroid

Center

DenisCarriere · 2017-05-28T17:17:57Z

As for scaling each feature separately in a FeatureCollection, it's an interesting approach but I'm wondering what you expect the use case to be?

@dpmcmlxxvi If you provide a FeatureCollection using the bare minimum parameters scale(geojson, 2), what would you expect?

I personally would expect this:

DenisCarriere · 2017-05-28T17:38:50Z

@dpmcmlxxvi @stebogit Good news!! No issues up in the Northern Latitudes (~75-80 lat)

polygon-resolute-bay.geojson

Bad (expected) news, some issues with a Fiji MultiPolygon.

We can ignore this for now, but another day we look for a solution.

polygon-fiji.geojson

stebogit · 2017-05-28T17:43:20Z

To me the difference between FeatureCollection and Multi(Feature) is the relationship between the features: the first case we have a collection of independent features (FeatureCollection is like a container for the features), while the second is actually a single feature described by multiple similar features. Therefore I'd expect a transformation on a collection as independently performed.

DenisCarriere · 2017-05-28T17:45:52Z

@stebogit Agreed, FeatureCollection is simply a container of Features which scale should be applied to each individual feature. MultiFeature should be treated as an entire object when using Scale (not to use flatten transformation).

DenisCarriere · 2017-05-28T17:47:11Z

The FeatureCollection handling logic is pretty straightforward.

// Scale each Feature separately
if (geojson.type === 'FeatureCollection') {
    featureEach(geojson, function (feature, index) {
        geojson.features[index] = scale(feature, factor, origin);
    });
    return geojson;
}
// Scale Feature/Geometry
return scale(geojson, factor, origin);

dpmcmlxxvi · 2017-05-28T17:51:03Z

@DenisCarriere I guess when I think of a scaling operation I normally expect there to be one fixed origin from which to scale the entire dataset. However, I'm fine with the current implementation, as long as the documentation is clear on what it is doing, because if the user wants a scaling from a single origin they can just provide a Point as the origin, correct?

stebogit · 2017-05-28T18:08:22Z

@DenisCarriere @dpmcmlxxvi I guess we'd need this then (note originIsPoint is just a logical evaluation):

// Scale each Feature separately
if (geojson.type === 'FeatureCollection' && !originIsPoint) {
    featureEach(geojson, function (feature, index) {
        geojson.features[index] = scale(feature, factor, origin);
    });
    return geojson;
}
// Scale Feature/Geometry
return scale(geojson, factor, origin);

However this behaviour would need to be clearly documented, as I don't think is obvious

dpmcmlxxvi · 2017-05-28T18:11:12Z

@stebogit Actually, I think the current implementation handles that case as-is, it is just scaling the coordinates of each feature separately, as opposed to the coordinates of the entire collection, but either way it should be the same effect since the origin is the same.

DenisCarriere · 2017-05-28T20:53:15Z

However this behaviour would need to be clearly documented, as I don't think is obvious

Agreed, would need to clarify the FeatureCollection behavior.

@stebogit Added !originIsPoint 👍 Good call

stebogit · 2017-05-29T03:26:41Z

@DenisCarriere the 180th Meridian problem is fascinating, I didn't know about it!
Clearly we need to work on turf-rhumb-destination (which of course presents the same issue) and once fixed that we should have fixed also all the transformations, all affected.
I'll create an issue and work on it right away.

DenisCarriere · 2017-05-29T03:51:41Z

👍 Yes the 180th Meridian problem is a fun one! Don't sweat it too much since it's a common problem for A LOT of GIS applications to handle. Especially once you start handing a Geometry which spans on both sides of the 180th Meridian (50km buffer around Fiji is a good example of that).

stebogit · 2017-05-29T17:30:01Z

@DenisCarriere @dpmcmlxxvi I guess once we merge #770 and re-test everything we're good to merge this. Anything else missing?
Once done here I'll test the other transform modules against the 180th meridian issue.

DenisCarriere · 2017-05-29T18:18:45Z

Should be good to go! We can update the fixtures once both PRs are in the master branch.

stebogit added 2 commits May 27, 2017 00:46

first implementation, incorrect output...

d077969

fixed types

7239acd

stebogit added the new-module label May 27, 2017

stebogit self-assigned this May 27, 2017

stebogit requested a review from DenisCarriere May 27, 2017 08:30

Drop tap from packages testing

9ca036e

Tap will be used at the root level and not in the individual repos

DenisCarriere requested changes May 27, 2017

View reviewed changes

DenisCarriere reviewed May 27, 2017

View reviewed changes

Minor refactoring

2463159

DenisCarriere approved these changes May 27, 2017

View reviewed changes

stebogit commented May 27, 2017

View reviewed changes

dpmcmlxxvi reviewed May 27, 2017

View reviewed changes

stebogit commented May 27, 2017

View reviewed changes

DenisCarriere reviewed May 27, 2017

View reviewed changes

Add defineOrigin method

6015aa6

CC: @stebogit

stebogit commented May 27, 2017

View reviewed changes

DenisCarriere added 2 commits May 27, 2017 16:05

Add more testing

aec2e1c

Update readme

a1698d0

stebogit added 2 commits May 28, 2017 01:19

updated benchmark

532efe8

removed 'scaleFactor';

af73c1c

added tests with origin as point;

DenisCarriere added 2 commits May 28, 2017 11:58

Add line end of line

8457824

Add Feature Collection to tests

1853fd7

Scale FeatureCollection separately

237c8a6

As per #759 (comment)

Add Fiji & Resolute bay to test fixtures

ce3e9c8

Update Docs for FeatureCollection

b4ae54d

DenisCarriere merged commit e0c42bb into master May 29, 2017

DenisCarriere deleted the transform-scale branch May 29, 2017 18:19

DenisCarriere mentioned this pull request May 29, 2017

New module proposal: Transform #747

Closed

6 tasks

DenisCarriere added this to the 4.4.0 milestone May 29, 2017


		# scale

		Moves any geojson Feature or Geometry of a specified distance along a Rhumb Line

New module @turf/transform-scale #759

New module @turf/transform-scale #759

Conversation

stebogit commented May 27, 2017

DenisCarriere commented May 27, 2017

DenisCarriere commented May 27, 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

DenisCarriere commented May 27, 2017 • edited Loading

stebogit commented May 27, 2017

DenisCarriere commented May 27, 2017

stebogit May 27, 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

DenisCarriere May 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpmcmlxxvi commented May 27, 2017

dpmcmlxxvi commented May 27, 2017

Choose a reason for hiding this comment

stebogit May 27, 2017 • edited Loading

Choose a reason for hiding this comment

stebogit commented May 27, 2017

stebogit commented May 27, 2017 • edited Loading

dpmcmlxxvi commented May 27, 2017

stebogit May 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stebogit commented May 28, 2017

DenisCarriere commented May 28, 2017 • edited Loading

DenisCarriere commented May 28, 2017 • edited Loading

Handling FeatureCollection

Proposed (Multiple centroids per feature)

Before (Single centroid origin)

DenisCarriere commented May 28, 2017 • edited Loading

dpmcmlxxvi commented May 28, 2017

DenisCarriere commented May 28, 2017

Centroid

Center

DenisCarriere commented May 28, 2017

DenisCarriere commented May 28, 2017 • edited Loading

stebogit commented May 28, 2017

DenisCarriere commented May 28, 2017

DenisCarriere commented May 28, 2017

dpmcmlxxvi commented May 28, 2017 • edited Loading

stebogit commented May 28, 2017

dpmcmlxxvi commented May 28, 2017

DenisCarriere commented May 28, 2017 • edited Loading

stebogit commented May 29, 2017

DenisCarriere commented May 29, 2017

stebogit commented May 29, 2017

DenisCarriere commented May 29, 2017

DenisCarriere commented May 27, 2017 •

edited

Loading

DenisCarriere commented May 27, 2017 •

edited

Loading

stebogit May 27, 2017 •

edited

Loading

DenisCarriere May 27, 2017 •

edited

Loading

stebogit May 27, 2017 •

edited

Loading

stebogit commented May 27, 2017 •

edited

Loading

stebogit May 27, 2017 •

edited

Loading

DenisCarriere commented May 28, 2017 •

edited

Loading

DenisCarriere commented May 28, 2017 •

edited

Loading

DenisCarriere commented May 28, 2017 •

edited

Loading

DenisCarriere commented May 28, 2017 •

edited

Loading

dpmcmlxxvi commented May 28, 2017 •

edited

Loading

DenisCarriere commented May 28, 2017 •

edited

Loading