New module @turf/sector #653

stebogit · 2017-04-09T07:10:27Z

@DenisCarriere I implemented the turf-sector module (related to #155 and #649).
However I did not create the README file cause I thought it was created automatically from the documentation in the index.js, but it didn't. Should I create it manually?
Not sure if the index.d.ts and bench.js are correct, as I'm not familiar with them.

Is that ok if I created the branch in this repo or should I work on my forked one?

Adraesh · 2017-04-09T07:31:50Z

Thank for the fast implementation.

Is it already ready in the turf.min.js ?

Thanks.

stebogit · 2017-04-09T07:42:34Z

@Adraesh I just happened to be implementing it at the right moment apparently 😄
It is not yet an official module, just created the PR. But you might want to try it as npm module and see if you find any issue with it.
BTW I don't really know how the process works to have it in the min version.

DenisCarriere · 2017-04-09T16:04:09Z

However I did not create the README file cause I thought it was created automatically from the documentation in the index.js, but it didn't. Should I create it manually?

It is created by the index.js, however when creating a new modules you would need to initially run /scripts/generate-readme.

Not sure if the index.d.ts and bench.js are correct, as I'm not familiar with them.

I can help review the Typescript definition.
Benchmark is useful to determine the efficiency of the module, you can almost copy-paste any recent module for the benchmark template (ex: turf-bbox-clip

Is that ok if I created the branch in this repo or should I work on my forked one?

Perfectly fine what you did, for every new PR generate a new forked branches from Turf's master branch.

BTW I don't really know how the process works to have it in the min version.

The minified build happens when a new release is published using npm build.

CC: @stebogit

DenisCarriere · 2017-04-09T16:13:00Z

@stebogit Look over your source code! 👍 I'm going to send over a few commits to add dynamic testing from GeoJSON point features in test/in & test/out.

stebogit · 2017-04-09T17:05:10Z

@DenisCarriere so this is the way to go for any (new) module now, right?

dynamic testing from GeoJSON point features in test/in & test/out.

I started this module by cloning the circle module which doesn't have that, so I though the test/in - test/out thing was only used for more complex modules

DenisCarriere · 2017-04-09T17:40:16Z

@stebogit ideally every module should have dynamic testing and you shouldn't have to touch the source code of the test.js, simply add geojson features to the test/in is the easiest way that others can contribute to improve the test cases. Also it's impossible to visually see the results when they are not being generated in test/out.

Just added dynamic tests via 57b0717.

Noticed an issue with:

bearing1= -50
bearing2= -15

And an error with:

bearing1= -15
bearing2= -50

Error: Each LinearRing of a Polygon must have 4 or more Positions.
    at module.exports.polygon (/Users/mac/Github/turf-sector/packages/turf-sector/node_m
odules/@turf/helpers/index.js:84:19)

DenisCarriere · 2017-04-09T17:45:04Z

This might be handy to have 65c3c36

~~extract required params from GeoJSON properties (only applied to required params and must be undefined)~~ (Edit: not a common behavior in TurfJS, i'll remove in next commit)
more verbose validation (to know exactly which param required)

Different OS has minor differences in coordinate decimals, truncate normalizes the results to pass on all different OS

stebogit · 2017-04-09T19:11:18Z

@DenisCarriere I don't understand exactly how to run ./scripts/generate-readmes, would you mind adding some more details about this on CONTRIBUTING.md, I think it might be beneficial for other less expert developers, like me, as well.

DenisCarriere · 2017-04-09T19:49:38Z

@stebogit Are you running on MacOS or Linux? You can simply just execute it via the terminal/command line.

$ ./scripts/generate-readmes

In windows you might need to add node in front of the script

$ node ./scripts/generate/readmes

DenisCarriere · 2017-04-09T21:28:38Z

@stebogit You might be interested to know that there's already a getCoords() function in @turf/invariant, it's an easy way to get coords from Geometry or Feature or an Array.

https://github.com/Turfjs/turf/blob/master/packages/turf-invariant/index.js#L22-L50

@stebogit

CC: @stebogit

@stebogit

This `sector-error.geojson` seems to be having some issues, it should be the inverse. - bearing1 = -15 (345) - bearing2 = -50 (310) CC: @stebogit

DenisCarriere · 2017-04-09T21:54:11Z

Added test case sector-error.geojson seems to be having some issues, it should be the inverse.

bearing1 = -15 (345)
bearing2 = -50 (310)

I'm sure this would be resolved if we have a 100% tested @turf/line-arc module based on your getArcLine function.

@stebogit 👍 We are almost there!!

stebogit · 2017-04-10T15:13:08Z

packages/turf-sector/bench.js

+ * sector3 x 38,093 ops/sec ±2.97% (77 runs sampled)
+ * sector4 x 210,468 ops/sec ±2.58% (78 runs sampled)
+ * sector5 x 26,438 ops/sec ±9.98% (70 runs sampled)
+ * sector6 x 29,032 ops/sec ±2.36% (70 runs sampled)
 */


@DenisCarriere I thought to run bench.js and it turned out it worked 😆 (I guess)
I don't understand though if the result is good, I mean how do I evaluate these numbers, what do they mean?

Well the results will vary based on each machine, however it's a good performance gauge when you are trying to tweak a specific function.

Let's say you want to know if map() is faster than featureEach(), you can run the benchmark and then change the function and run the benchmark again to see which one has better performance.

Also when refactoring existing code, you might want to know if your "new" code is actually improving the performance or decreasing it. My @turf/buffer attempt is a good example of refactoring gone bad 🤦‍♂️ #656 (fail me)

so higher ops/sec means better performance, right?
then you might want to re-run bench.js here, as I'm sure my old macbook air here at home is slower than your machine...

Yes higher ops/sec is better.

The Benchmark results are approximate, it gives you a good idea where the module stands.

stebogit · 2017-04-10T15:19:53Z

@DenisCarriere since we have now more than one module having angles/bearing as input, I think the convertAngleTo360 function (used in both turf-sector and turf-line-arc) could be moved into turf-helpers, what do you think?
Maybe you can find a better name.

DenisCarriere · 2017-04-10T15:31:02Z

@stebogit 👍 for convertAngleTo360 that would fit the theme of @turf/helpers, there's already a few degrees methods https://github.com/Turfjs/turf/blob/master/packages/turf-helpers/index.js#L277-L320.

I'm sure there's a few other TurfJS modules that have this type of function internally built in, you could poke around and see if you could include it in other modules.

stebogit · 2017-04-10T15:42:37Z

@DenisCarriere may/should I add this task as a new issue and assign it to myself then?
Are you ok with the name of the function? I'm not 100% satisfied, but I can't find really a better one...

DenisCarriere · 2017-04-10T15:57:23Z

@stebogit Yep, make a new issue or open up a new PR and briefly explain it.

Usually I let a PR sit for a few days and review it again with a fresh mindset, you might find a better name for convertAngleTo360 in a couple days.

stebogit · 2017-04-11T02:59:08Z

@DenisCarriere what about bearingToAngle?
After some research I find out that the 'bearing' is the angle between (generally) the north and a generic line/direction (that's why bearing is -180 and +180 degrees!), while an angle is generally express between 0 and 360 degrees (here again we might choose north as reference).
This is probably obvious to most people, but I would add some details in the turf-bearing description just to be more clear.

DenisCarriere · 2017-04-11T04:05:35Z

@stebogit 👍 I like it a lot! A lot better than the previous method name.

I think you've got a winner there!

stebogit · 2017-04-11T15:22:16Z

@DenisCarriere I believe this module might be ready for merging, after your blessing 😃

DenisCarriere · 2017-04-12T04:29:43Z

@stebogit ✋ High Five! Looks great!

DenisCarriere · 2017-04-12T04:45:27Z

GeoJSON Pac-Man! 🚀

https://github.com/Turfjs/turf/blob/master/packages/turf-sector/test/out/pacman.geojson

stebogit · 2017-04-12T05:25:08Z

@DenisCarriere Great, thanks! 🎉
How come that the branch turf-sector is still unmerged? Did you just copied the module over to master? Just curious

DenisCarriere · 2017-04-12T05:31:24Z

Strange, might be an issue on the GitHub Network graph. I simply merged it via this PR, nothing special (maybe check again tomorrow, might be a delay).

Regardless, it's always best to delete the branch after it's been merged (if no more work is going to happen on that branch).

DenisCarriere · 2017-04-12T13:14:29Z

@stebogit Reference the network graph. Maybe it's because when I merge I always select the Squash and merge vs. using Create a merge commit (I personally like having a single commit for each individual PRs vs. having 10 new commits for one fix/new module).

DenisCarriere · 2017-04-12T13:46:50Z

Yep that was it, I just did a merge using Create a merge commit with bearingToAngle and the network graph merged.

added turf-sector module

a13e0c9

DenisCarriere assigned stebogit Apr 9, 2017

DenisCarriere added the new-module label Apr 9, 2017

DenisCarriere added this to the 4.1.0 milestone Apr 9, 2017

Add dynamic geojson testing

57b0717

extract required params from GeoJSON properties

65c3c36

DenisCarriere added 5 commits April 9, 2017 13:56

Add benchmark

4a9db06

truncate coordinate decimals

01e0ed2

Different OS has minor differences in coordinate decimals, truncate normalizes the results to pass on all different OS

Remove extract required params from properties

a214c4a

Update benchmark results

2cbeeae

Update Typescript defintion with distance Units

4da5c5a

DenisCarriere changed the title ~~added turf-sector module~~ New module @turf/sector Apr 9, 2017

DenisCarriere changed the title ~~New module @turf/sector~~ New module @turf/sector Apr 9, 2017

DenisCarriere added 3 commits April 9, 2017 17:30

Add getCoords from invariant

776b1fb

CC: @stebogit

Fix sector4 test

8bbc414

Add test case with error

bd09e62

This `sector-error.geojson` seems to be having some issues, it should be the inverse. - bearing1 = -15 (345) - bearing2 = -50 (310) CC: @stebogit

stebogit added 3 commits April 9, 2017 23:58

corrected bearing2 error message

a820d44

fixed error with bearing1 > bearing2

36ea041

removed debug variable

99b4f95

DenisCarriere modified the milestones: 4.2.0, 4.1.0 Apr 10, 2017

stebogit added 2 commits April 10, 2017 07:56

Merge branch 'master' into turf-sector

fd37730

replaced getArcLine() with @turf/line-arc

9f7331c

stebogit commented Apr 10, 2017

View reviewed changes

DenisCarriere merged commit 6dd8e9f into master Apr 12, 2017

DenisCarriere deleted the turf-sector branch April 12, 2017 05:31

DenisCarriere restored the turf-sector branch April 12, 2017 05:32

DenisCarriere deleted the turf-sector branch April 12, 2017 05:32

DenisCarriere mentioned this pull request Apr 30, 2017

New minor release! Turf 4.2.0 🎉 #697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New module @turf/sector #653

New module @turf/sector #653

stebogit commented Apr 9, 2017

Adraesh commented Apr 9, 2017

stebogit commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017 •

edited

Loading

DenisCarriere commented Apr 9, 2017

stebogit commented Apr 9, 2017 •

edited

Loading

DenisCarriere commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017 •

edited

Loading

stebogit commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017 •

edited

Loading

stebogit Apr 10, 2017

DenisCarriere Apr 10, 2017 •

edited

Loading

stebogit Apr 10, 2017

DenisCarriere Apr 10, 2017

stebogit commented Apr 10, 2017

DenisCarriere commented Apr 10, 2017

stebogit commented Apr 10, 2017 •

edited

Loading

DenisCarriere commented Apr 10, 2017

stebogit commented Apr 11, 2017

DenisCarriere commented Apr 11, 2017

stebogit commented Apr 11, 2017

DenisCarriere commented Apr 12, 2017

DenisCarriere commented Apr 12, 2017

stebogit commented Apr 12, 2017 •

edited

Loading

DenisCarriere commented Apr 12, 2017 •

edited

Loading

DenisCarriere commented Apr 12, 2017

DenisCarriere commented Apr 12, 2017

New module @turf/sector #653

New module @turf/sector #653

Conversation

stebogit commented Apr 9, 2017

Adraesh commented Apr 9, 2017

stebogit commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017 • edited Loading

DenisCarriere commented Apr 9, 2017

stebogit commented Apr 9, 2017 • edited Loading

DenisCarriere commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017 • edited Loading

stebogit commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017

DenisCarriere commented Apr 9, 2017 • edited Loading

stebogit Apr 10, 2017

Choose a reason for hiding this comment

DenisCarriere Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

stebogit Apr 10, 2017

Choose a reason for hiding this comment

DenisCarriere Apr 10, 2017

Choose a reason for hiding this comment

stebogit commented Apr 10, 2017

DenisCarriere commented Apr 10, 2017

stebogit commented Apr 10, 2017 • edited Loading

DenisCarriere commented Apr 10, 2017

stebogit commented Apr 11, 2017

DenisCarriere commented Apr 11, 2017

stebogit commented Apr 11, 2017

DenisCarriere commented Apr 12, 2017

DenisCarriere commented Apr 12, 2017

GeoJSON Pac-Man! 🚀

stebogit commented Apr 12, 2017 • edited Loading

DenisCarriere commented Apr 12, 2017 • edited Loading

DenisCarriere commented Apr 12, 2017

DenisCarriere commented Apr 12, 2017

DenisCarriere commented Apr 9, 2017 •

edited

Loading

stebogit commented Apr 9, 2017 •

edited

Loading

DenisCarriere commented Apr 9, 2017 •

edited

Loading

DenisCarriere commented Apr 9, 2017 •

edited

Loading

DenisCarriere Apr 10, 2017 •

edited

Loading

stebogit commented Apr 10, 2017 •

edited

Loading

stebogit commented Apr 12, 2017 •

edited

Loading

DenisCarriere commented Apr 12, 2017 •

edited

Loading