New module @turf/ellipse - Create an Ellipse submodule based on Circle #1087

muziejus · 2017-11-11T08:02:12Z

Comments:

I haven't changed the index.d.ts file because I don't know what it does.

Please fill in this template.

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 occurred.
Run npm run lint to ensure code style at the turf module level.

Submitting a new TurfJS Module.

Overview description of proposed module.
Include JSDocs with a basic example.
Execute ./scripts/generate-readmes to create README.md.
Add yourself to contributors in package.json using "Full Name <@github Username>".

…object.

DenisCarriere · 2017-11-11T19:17:18Z

Nice! Thanks for the contributions! Going to have a look at this more closely later on.

I'll help out to make the TravisCI tests pass.

muziejus · 2017-11-11T20:46:36Z

Thanks. My goal, if it's not obvious, is to add Standard Deviational Ellipse functionality to turf. The potential for geostatistical packages in interactive maps (in leaflet, say)… 💥

DenisCarriere · 2017-11-11T21:26:03Z

When creating an Ellipse, would it also be possible points (Coordinates) to define xRadius & yRadius... 🤔 might be tricky, but I think that would be possible using @turf/distance.

DenisCarriere

👍 Looks good, I've left a few comments, I can help with any final touches.

DenisCarriere · 2017-11-11T21:16:07Z

packages/turf-ellipse/index.js

+ * //addToMap
+ * const addToMap = [turf.point(center), ellipse]
+ */
+module.exports = function (center, xRadius, yRadius, options) {


We are using ES Modules now + this type of module should also be using export default.

function ellipse(center, xRadius, yRadius, options) { ... } export default ellipse

DenisCarriere · 2017-11-11T21:17:01Z

packages/turf-ellipse/index.js

+
+/**
+ * Takes a {@link Point} and calculates the ellipse polygon given two axes in degrees and steps for precision.
+ *


Most modules will contain a @name JSDocs tag.

/** * @name ellipse */

DenisCarriere · 2017-11-11T21:18:12Z

packages/turf-ellipse/index.js

@@ -0,0 +1,68 @@
+const polygon = require('@turf/helpers').polygon;
+const lengthToDegrees = require('@turf/helpers').lengthToDegrees;
+const getCoord = require('@turf/invariant').getCoord;


Since Turf now uses ES Modules, you should be using the import syntax.

import { polygon, lengthToDegrees } from '@turf/helpers'; import { getCoord } from '@turf/invariant';

DenisCarriere · 2017-11-11T21:19:56Z

packages/turf-ellipse/index.d.ts

+/**
+ * http://turfjs.org/docs/#ellipse
+ */
+declare function ellipse(feature1: Feature, feature2: Feature): boolean;


... 🤔 I know Typescript is new for a lot of folks, but you could simply copy the Typescript definition from @turf/circle which will be very similar to the new ellipse module.

https://github.com/Turfjs/turf/blob/master/packages/turf-circle/index.d.ts

DenisCarriere · 2017-11-11T21:27:01Z

packages/turf-ellipse/index.js

+    if (!center) throw new Error('center is required');
+    if (!xRadius) throw new Error('xRadius is required');
+    if (!yRadius) throw new Error('yRadius is required');
+    if (typeof options !== 'object') throw new Error('options must be an object');


There's a helper function isObject in @turf/helpers instead of using typeof.

DenisCarriere · 2017-11-11T21:28:25Z

packages/turf-ellipse/index.js

+    if (typeof options !== 'object') throw new Error('options must be an object');
+    if (typeof steps !== 'number') throw new Error('steps must be a number');
+
+    const centerCoords = getCoord(center);


At the moment TurfJS is only using ES5 code, ES6 is not being compiled to ES5 for the final build (at the moment).

Replace const with var

DenisCarriere · 2017-11-11T21:28:52Z

packages/turf-ellipse/index.js

+    xRadius = lengthToDegrees(xRadius, units);
+    yRadius = lengthToDegrees(yRadius, units);
+
+    let coordinates = [];


Same as above, replace let with var

DenisCarriere · 2017-11-11T21:30:20Z

packages/turf-ellipse/package.json

@@ -0,0 +1,43 @@
+{
+  "name": "@turf/ellipse",
+  "version": "5.0.4",


Could you change the version to 5.0.0 for the first publish, Lerna will decide the patch version.

DenisCarriere · 2017-11-11T21:30:43Z

packages/turf-ellipse/package.json

+  "types": "index.d.ts",
+  "files": [
+    "index.js",
+    "index.d.ts"


Missing main.js that will be the CommonJS distributed file from Rollup

DenisCarriere · 2017-11-11T21:31:29Z

packages/turf-ellipse/package.json

+    "index.d.ts"
+  ],
+  "scripts": {
+    "test": "node test.js",


Use the same scripts as all the other modules.

"scripts": { "pretest": "rollup -c ../../rollup.config.js", "test": "node -r @std/esm test.js", "bench": "node -r @std/esm bench.js" },

You'll also need to install @std/esm with the following config:

"@std/esm": { "esm": "js", "cjs": true }

DenisCarriere · 2017-11-11T22:09:07Z

I'm looking at the ellipse code base, why don't we simply add xRadius & yRadius as options to @turf/circle?

DenisCarriere · 2017-11-11T22:13:46Z

My goal, if it's not obvious, is to add Standard Deviational Ellipse functionality to turf. The potential for geostatistical packages in interactive maps (in leaflet, say)…

👍 Super! 🚀 Lets definitely get this done, I think expanding the @turf/circle module will be our best bet. I'll set up a PR to get us started.

muziejus · 2017-11-11T23:37:50Z

@DenisCarriere Thanks for making these changes. A few notes:

Abstractly:

I don't think adding a second radius to circle will do the trick, because circle uses distance to plot the points on the circle, but the distance is variable in an ellipse, relying on the interplay between the two radii. I think the more traditional means of measuring w/ tangents works great. Whether that should be abstracted to its own helper, on the other hand…
I don't think we can use points instead of distances for the radii because the radii have to be perpendicular to each other.

Concretely:

Some of the lack of packages, etc., was a function of how the default new submodule script works. I'm not very familiar w/ node, so this required some guess work.
I stayed away from using import, etc., because they were simply breaking my tests:

/Users/moacir/Projects/turf/packages/turf-ellipse/test.js:1
(function (exports, require, module, __filename, __dirname) { import test from 'tape';
                                                              ^^^^^^

SyntaxError: Unexpected token import

Switching to require() syntax fixed that.

muziejus

Looks great. Thanks! 👍 I left a comment addressing come of your questions.

stebogit · 2017-11-12T08:29:33Z

@muziejus @DenisCarriere nice addition here. 👍

Are the radii required by the module actually the semi-minor and the semi-major axes of the ellipse? If so I'd describe that way in the docs.

May I suggest to include also an optional tilt parameter related to the major axis of the ellipse for example? You could probably just apply @turf/transform-rotate at the end if the parameter (angle) is defined.

I don't think having this as a separate module would be a terrible idea, it would make it "easier to find" or like more explicit that Turf has that functionality. Just a thought.

muziejus · 2017-11-12T17:51:56Z

@stebogit You're right; they are the semi-minor and semi-major axes. My terminology is bad. But we can't use terms like "major" and "minor" because they actually relate to the x and y--it's very easy to send, say, ellipse(place, 2, 10); and end up with an ellipse where the semi-major axis is along the y-axis.

Maybe xSemiAxis and ySemiAxis?

muziejus · 2017-11-12T17:56:26Z

@stebogit As for tilting, I considered that, since rotation is a vital aspect of the standard deviational ellipse, but given there's already a diff utility, I don't think adding another parameter is necessary.

If I were to add a tilt option, why not also add a translate or scale option, etc.?

stebogit · 2017-11-12T18:55:33Z

@muziejus looking at the picture in my understanding we are passing O (as a point), a and b (as lengths); with those and a tilt angle (𝛼), you can describe the ellipse in the picture. In fact the center defines the position (no need for a translate parameter) and the axes define the dimensions (no need for a scale param). However without a tilt you would actually define the gray ellipse in the picture (where a=c and b=d).
Unless we always want to create ellipses with axes parallel to the x and y axes (is this actually the case?), I'd say we need a tilt.

Maybe xSemiAxis and ySemiAxis?

We could easily use major and minor axes and internally divide them by 2

there's already a diff utility

What do you mean?

muziejus · 2017-11-12T20:10:55Z

@stebogit You're right. I meant "separate utility" in that the user could always use transformRotate() on their own after the fact, but I don't see why I should be adamant about not adding the functionality into this submodule.

DenisCarriere · 2017-11-17T07:54:39Z

✅ 👍 for adding tilt as an option to this module.

✅ 👍 using major & minor.

✅ 👍 Creating it's own module @turf/ellipse

❌ 👎 add a translate or scale option (doesn't seem necessary given there's already 3 parameters that will allow to control that)

It is best to create it's own dedicated module @turf/ellipse like @stebogit mentioned, it would be easier to find and it would make it easier to understand the context of the major & minor options given it belongs to a module called ellipse.

DenisCarriere

Approved once the module supports at least the following 3 params & 2 options:

Params

center
major
minor

Options

tilt
units

DenisCarriere · 2017-11-17T08:00:29Z

🤔 Seems like the build is failing because of a linting issue.

/home/travis/build/Turfjs/turf/packages/turf-ellipse/index.js
  50:13  error  'angle' is already defined                    no-redeclare
  66:1   error  Expected indentation of 4 spaces but found 0  indent
✖ 2 problems (2 errors, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

DenisCarriere · 2017-11-17T08:40:25Z

Cannot find module @turf/transform-rotate

Missing dependency 😄 The joys... Thank you TravisCI for being there for us.

lerna ERR! Error [ERR_MISSING_MODULE]: Cannot find module @turf/transform-rotate
lerna ERR!     at Object.<anonymous> (/home/travis/build/Turfjs/turf/packages/turf-ellipse/main.js:5:23)
lerna ERR!     at Object.<anonymous> (/home/travis/build/Turfjs/turf/packages/turf-ellipse/main.js)
lerna ERR!     at Module._compile (module.js:641:30)
lerna ERR! npm ERR! code ELIFECYCLE

DenisCarriere · 2017-11-18T21:20:32Z

👍 Awesome stuff @muziejus!

Moacir P. de Sá Pereira added 13 commits November 11, 2017 00:49

Initial run of create-new-module

78dfa3b

Add helpers to package.

72df84b

functioning basic function.

b096932

separate out helper function

7c42fdb

Remove section on default params and move up

1dd43f8

Change center to Washington Sq. Park :)

783dec4

Allow an array of coordinates as the center as well as a turf.points …

edd1471

…object.

Add mapbox to test geojson.

15a4aba

Add units as a parameter.

73bb74f

Generate README.

b072cd4

Add benchmarking.

596590d

Add my name to the package.json.

b3ee69b

Rewind package.json to 5.0.4

c16a3ce

Change xAxis and yAxis to xRadius and yRadius

1378243

Merge branch 'master' into ellipse

8fdb605

DenisCarriere requested changes Nov 11, 2017

View reviewed changes

DenisCarriere added 2 commits November 11, 2017 17:00

Update Script & DevDependencies

4c4c9f9

Convert to ES5

a992e03

DenisCarriere added 2 commits November 11, 2017 17:10

Update index.d.ts

0ed6c20

Remove extra semi-colons

698653b

DenisCarriere and others added 5 commits November 11, 2017 17:17

Update package.json

f5c528a

Convert tests to ES6 modules

f3266aa

Add std/esm

ee38807

Update package.json

c0de0b2

Give better description of the function--it takes radii not axes

948a772

muziejus commented Nov 11, 2017

View reviewed changes

Moacir P. de Sá Pereira and others added 3 commits November 12, 2017 13:08

change “Radius” to “SemiAxis”

93ea216

Update CHANGELOG.md

29e7c63

Update CHANGELOG.md

49462c5

muziejus force-pushed the ellipse branch from 365ce93 to 49462c5 Compare November 12, 2017 18:08

Add rotation and pivoting.

e9d4aac

Merge branch 'master' of github.com:Turfjs/turf into ellipse

d5777d1

Merge branch 'master' into ellipse

aec2ab3

DenisCarriere added the new-module label Nov 17, 2017

DenisCarriere added this to the 5.1.0 milestone Nov 17, 2017

DenisCarriere assigned muziejus Nov 17, 2017

DenisCarriere approved these changes Nov 17, 2017

View reviewed changes

Moacir P. de Sá Pereira added 2 commits November 17, 2017 03:27

Merge branch 'ellipse' of github.com:muziejus/turf into ellipse

f2ef418

Fix linting errors.

5ba761d

Moacir P. de Sá Pereira added 3 commits November 17, 2017 03:45

Add rotate to list of dependencies.

2598ae9

Fix importing.

e480379

Fix tests to take into account default distance of kilometers...

7820a84

DenisCarriere merged commit 04480be into Turfjs:master Nov 18, 2017

DenisCarriere changed the title ~~Create an Ellipse submodule based on Circle~~ New module @turf/ellipse - Create an Ellipse submodule based on Circle Nov 27, 2017

DenisCarriere mentioned this pull request Dec 7, 2017

New minor release! Turf 5.1.0 🎉 #1162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New module @turf/ellipse - Create an Ellipse submodule based on Circle #1087

New module @turf/ellipse - Create an Ellipse submodule based on Circle #1087

muziejus commented Nov 11, 2017

DenisCarriere commented Nov 11, 2017

muziejus commented Nov 11, 2017

DenisCarriere commented Nov 11, 2017

DenisCarriere left a comment •

edited

Loading

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere Nov 11, 2017

DenisCarriere commented Nov 11, 2017

DenisCarriere commented Nov 11, 2017

muziejus commented Nov 11, 2017

muziejus left a comment

stebogit commented Nov 12, 2017

muziejus commented Nov 12, 2017

muziejus commented Nov 12, 2017

stebogit commented Nov 12, 2017

muziejus commented Nov 12, 2017

DenisCarriere commented Nov 17, 2017

DenisCarriere left a comment •

edited

Loading

DenisCarriere commented Nov 17, 2017

DenisCarriere commented Nov 17, 2017 •

edited

Loading

DenisCarriere commented Nov 18, 2017

New module @turf/ellipse - Create an Ellipse submodule based on Circle #1087

New module @turf/ellipse - Create an Ellipse submodule based on Circle #1087

Conversation

muziejus commented Nov 11, 2017

DenisCarriere commented Nov 11, 2017

muziejus commented Nov 11, 2017

DenisCarriere commented Nov 11, 2017

DenisCarriere left a comment • 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

DenisCarriere commented Nov 11, 2017

DenisCarriere commented Nov 11, 2017

muziejus commented Nov 11, 2017

muziejus left a comment

Choose a reason for hiding this comment

stebogit commented Nov 12, 2017

muziejus commented Nov 12, 2017

muziejus commented Nov 12, 2017

stebogit commented Nov 12, 2017

muziejus commented Nov 12, 2017

DenisCarriere commented Nov 17, 2017

DenisCarriere left a comment • edited Loading

Choose a reason for hiding this comment

DenisCarriere commented Nov 17, 2017

DenisCarriere commented Nov 17, 2017 • edited Loading

DenisCarriere commented Nov 18, 2017

DenisCarriere left a comment •

edited

Loading

DenisCarriere left a comment •

edited

Loading

DenisCarriere commented Nov 17, 2017 •

edited

Loading