-
Notifications
You must be signed in to change notification settings - Fork 942
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
New segmentEach & segmentReduce (@turf/meta) #863
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 @DenisCarriere
I believe the only issue might be excluding (Multi)Point
as input.
packages/turf-meta/index.js
Outdated
var currentIndex = 0; | ||
flattenEach(geojson, function (feature) { | ||
coordReduce(feature, function (previousCoords, currentCoords) { | ||
var currentSegment = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere I bet it's a choice not to have dependencies in turf-meta
. 👍
Otherwise you could use
var currentSegment = lineString([previousCoords, currentCoords], feature.properties || {});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... @turf/meta
has no dependencies, otherwise I would use that
packages/turf-meta/index.js
Outdated
* @param {*} [initialValue] Value to use as the first argument to the first call of the callback. | ||
* @returns {void} | ||
* @example | ||
* var polygon = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the above example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No dependencies for meta
module, so for this one we can exclude the helpers
functions. If we change it, we should change it for the entire JSDoc's module
packages/turf-meta/index.js
Outdated
function segmentReduce(geojson, callback, initialValue) { | ||
var previousValue = initialValue; | ||
segmentEach(geojson, function (currentSegment, currentIndex) { | ||
if (currentIndex === 0 && initialValue === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks cleaner to me (learned from you 😜 ):
if (currentIndex === 0 && initialValue === undefined) previousValue = currentSegment;
else previousValue = callback(previousValue, currentSegment, currentIndex);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes & No.. :P since there's Sooo much going on for 1 line of code, they were kept separated.
But we can switch it to that, this got copy-pasted from the previous reduce
functions.
packages/turf-meta/index.js
Outdated
* @param {Function} callback a method that takes (currentSegment, currentIndex) | ||
* @returns {void} | ||
* @example | ||
* var polygon = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could use
var polygon = turf.polygon([[[-50, 5], [-40, -10], [-50, -10], [-40, 5], [-50, 5]]]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Agreed, I really do prefer these "shorter" examples, more focused on the code itself and not 10 lines of defining a GeoJSON polygon.
* total++; | ||
* }, initialValue); | ||
*/ | ||
function segmentEach(geojson, callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this gonna work on (Multi)Points
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works... but Multi Points aren't lines so no segments will be returned, we can include it in the test cases.
* return previousValue; | ||
* }, initialValue); | ||
*/ | ||
function segmentReduce(geojson, callback, initialValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, is it gonna work on (Multi)Point
? Probably we need to exclude them as valid inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to throw an error, because that would prevent having mixed inputs and it would be really aggravating having to pre-filter your data.
Points & MultiPoints are ignored which is defined in the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?? Oops, I thought I added that explanation in the JSDocs (about the (Multi)Point), I guess I must've forgotten about it.
👍 Good catch
@stebogit Added this to my 'To-Do'
|
- [x] Use of `turf.polygon` in JSDocs `@example` - [x] Compact If/Else statements - [x] Add explanation that (Multi)Point will be ignored - [x] Ignore (Multi)Point from being processed (should speed the iterating process). - [x] Add MultiPoint to tests - [x] Add internal `lineString` method CC: @stebogit
Thanks @stebogit for the review, I needed that 👍 |
* @param {Object} properties Properties | ||
* @returns {Feature<LineString>} GeoJSON LineString Feature | ||
*/ | ||
function lineString(coordinates, properties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere I don't get why exactly we need/want to have no dependencies in meta
, if we would actually need helpers
for instance. Is this faster maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful @DenisCarriere! As soon as you merge this to master I'll use these functions in @turf/boolean-overlap
!
* var initialValue = 0; | ||
* turf.segmentEach(polygon, function () { | ||
* total++; | ||
* }, initialValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere I believe initialValue
doesn't belong in here (only for segmentReduce
). Does it?
@DenisCarriere I believe in |
👍 Also we should test for the skipped indexes (ex: FeatureCollection of points & polygons) the point index will be skipped and not 1++.
|
@stebogit Implemented the |
New
segmentEach
&segmentReduce
(@turf/meta
)To-Do
segmentReduce
in@turf/line-distance
.turf.polygon
&turf.featureCollection
in JSDocs@example
lineString
methodsubIndex
segmentEach
segmentReduce
Benchmark
Based on a random Polygon