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

Add FeatureCollection mutate tests (not failing) #897

Merged
merged 7 commits into from
Aug 14, 2017
Merged
Show file tree
Hide file tree
Changes from 3 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
20 changes: 14 additions & 6 deletions packages/turf-transform-scale/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var meta = require('@turf/meta');
var clone = require('@turf/clone');
var center = require('@turf/center');
var helpers = require('@turf/helpers');
var centroid = require('@turf/centroid');
Expand All @@ -12,6 +13,7 @@ var coordEach = meta.coordEach;
var featureEach = meta.featureEach;
var getCoord = invariant.getCoord;
var getCoords = invariant.getCoords;
var getGeomType = invariant.getGeomType;


/**
Expand Down Expand Up @@ -39,17 +41,17 @@ module.exports = function (geojson, factor, origin, mutate) {
var originIsPoint = Array.isArray(origin) || typeof origin === 'object';

// Clone geojson to avoid side effects
if (mutate !== true) geojson = JSON.parse(JSON.stringify(geojson));
if (mutate !== true) geojson = clone(geojson);

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

/**
Expand All @@ -59,18 +61,24 @@ module.exports = function (geojson, factor, origin, mutate) {
* @param {Feature|Geometry} geojson GeoJSON Feature/Geometry
* @param {number} factor of scaling, positive or negative values greater than 0
* @param {string|Geometry|Feature<Point>|Array<number>} [origin="centroid"] Point from which the scaling will occur (string options: sw/se/nw/ne/center/centroid)
* @param {boolean} [mutate=false] allows GeoJSON input to be mutated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here mutate does not actually define if the feature is going to be mutated or not, that was handled before.
I can't find a good description though 😅

* @returns {Feature|Geometry} scaled GeoJSON Feature/Geometry
*/
function scale(geojson, factor, origin) {
function scale(geojson, factor, origin, mutate) {
// Default params
var isPoint = (geojson.type === 'Point' || geojson.geometry && geojson.geometry.type === 'Point');
var geomType = getGeomType(geojson);
var isPoint = geomType === 'Point';
var isPolygon = geomType === 'Polygon' || geomType === 'MultiPolygon';
origin = defineOrigin(geojson, origin);

// Shortcut no-scaling
if (factor === 1 || isPoint) return geojson;

// Scale each coordinate
coordEach(geojson, function (coord) {
coordEach(geojson, function (coord, coordIndex, featureIndex, featureSubIndex) {
Copy link
Collaborator

@stebogit stebogit Aug 12, 2017

Choose a reason for hiding this comment

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

I noticed coordIndex and featureSubIndex are always equals, throughout the entire test suite, and featureIndex is always 0.
Inside coordEach this never logs:
if (coordIndex !== featureSubIndex || featureIndex > 0) console.log('Woah!');
Shouldn't the indices change at least for multi features?

EDIT: Got it, geojson is a polygon with one ring, i.e. featureIndex = 0 and featureSubIndex = coordIndex

// Ignore scaling on first coordinate of Polygons (Issue #895)
if (isPolygon && mutate === true && featureSubIndex === 0) return;
Copy link
Member Author

Choose a reason for hiding this comment

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

@stebogit Might want to add this type of behavior in a few of the transform modules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DenisCarriere it looks ok, and clearly it does work. However I can not understand why we don't want to scale all the points of the ring(s) of a polygon, whether we clone it or not.
I really can't wrap my head around it... 🤔


var originalDistance = rhumbDistance(origin, coord);
var bearing = rhumbBearing(origin, coord);
var newDistance = originalDistance * factor;
Expand Down
2 changes: 2 additions & 0 deletions packages/turf-transform-scale/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
},
"homepage": "/~https://github.com/Turfjs/turf",
"devDependencies": {
"@turf/hex-grid": "^4.6.1",
"@turf/truncate": "^4.6.0",
"benchmark": "^2.1.4",
"load-json-file": "^2.0.0",
Expand All @@ -47,6 +48,7 @@
"@turf/bbox": "^4.6.0",
"@turf/center": "^4.6.1",
"@turf/centroid": "^4.6.1",
"@turf/clone": "^4.6.1",
"@turf/helpers": "^4.6.0",
"@turf/invariant": "^4.6.0",
"@turf/meta": "^4.6.0",
Expand Down
29 changes: 29 additions & 0 deletions packages/turf-transform-scale/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const path = require('path');
const load = require('load-json-file');
const write = require('write-json-file');
const center = require('@turf/center');
const hexGrid = require('@turf/hex-grid');
const truncate = require('@turf/truncate');
const turfBBox = require('@turf/bbox');
const centroid = require('@turf/centroid');
Expand Down Expand Up @@ -95,6 +96,34 @@ test('scale -- mutated input', t => {
t.end();
});

test('scale -- mutated FeatureCollection', t => {
const line = featureCollection([
lineString([[10, 10], [12, 15]]),
lineString([[15, 15], [22, 35]]),
lineString([[30, 30], [42, 45]])
]);
const lineBefore = JSON.parse(JSON.stringify(line));
scale(line, 1.5);
t.deepEqual(line, lineBefore, 'mutate = undefined - input should NOT be mutated');
scale(line, 1.5, 'centroid', false);
t.deepEqual(line, lineBefore, 'mutate = false - input should NOT be mutated');
scale(line, 1.5, 'centroid', 'nonBoolean');
t.deepEqual(line, lineBefore, 'non-boolean mutate - input should NOT be mutated');
t.end();
});

test('scale -- Issue #895', t => {
const grid = hexGrid([-122.930, 45.385, -122.294, 45.772], 5, 'miles');
featureEach(grid, (feature, index) => {
const factor = (index % 2 === 0) ? 0.4 : 0.6;
scale(feature, factor, 'centroid', true);
});
const output = directories.out + 'issue-#895.geojson';
if (process.env.REGEN) write.sync(output, grid);
t.deepEqual(grid, load.sync(output));
t.end();
});

test('scale -- geometry support', t => {
const pt = point([10, 10]);
const line = lineString([[10, 10], [12, 15]]);
Expand Down
Loading