-
Notifications
You must be signed in to change notification settings - Fork 944
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 turf-voronoi module for #1038 #1043
Conversation
packages/turf-voronoi/index.js
Outdated
import { lineString } from '@turf/helpers'; | ||
import d3voronoi from 'd3-voronoi'; | ||
|
||
function und3ify(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.
@stevage all the functions need a JDocs block, I bet that's why probably you're getting an error
packages/turf-voronoi/index.js
Outdated
* | ||
* The Voronoi algorithim used comes from the d3-voronoi package. | ||
* | ||
* @name voronoi |
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.
@stevage this should be turfVoronoi
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.
Are you sure about this? I've made this change, but Travis is failing and saying "not ok 12 voronoi is missing from index.js". Every other module I've looked at uses a short name that matches the function name.
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.
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.
@name voronoi
is good.
turfVoronoi
is incorrect.
packages/turf-voronoi/index.js
Outdated
* var voronoiPolygons = turf.voronoi(points, bbox); | ||
*/ | ||
function voronoi(points, bbox) { | ||
if (!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.
@stevage maybe we should instead throw an error here (since the parameter is required) or at least return "something", so it's clear that the input was empty.
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, both points
& bbox
should return Errors if not provided.
if (!points) throw new Error('points is required');
if (!bbox) throw new Error('bbox is required');
packages/turf-voronoi/index.js
Outdated
|
||
var polygonsd3 = d3v(pointsd3).polygons(); | ||
|
||
return { |
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.
@stevage you could import featureCollection
from helpers
and:
return featureCollection(polygonsd3.map(und3ify));
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.
👍
packages/turf-voronoi/index.js
Outdated
* The Voronoi algorithim used comes from the d3-voronoi package. | ||
* | ||
* @name voronoi | ||
* @param {FeatureCollection<Point>|Feature[]<Point>} points to find the Voronoi polygons around. |
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.
The input should only be FeatureCollection<Point>
, an Array of Features isn't a valid GeoJSON and a single point doesn't would most likely not be valid for this type of operation.
packages/turf-voronoi/index.js
Outdated
* @name voronoi | ||
* @param {FeatureCollection<Point>|Feature[]<Point>} points to find the Voronoi polygons around. | ||
* @param {number[]} bbox clipping rectangle, in [minX, minY, maxX, MaxY] order. | ||
* @returns {FeatureCollection<Polygon} a set of polygons, one per input 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.
Missing a closing bracket, that's most likely the issue with the JSDocs.
FeatureCollection<Polygon
=> FeatureCollection<Polygon>
packages/turf-voronoi/index.js
Outdated
* @returns {FeatureCollection<Polygon} a set of polygons, one per input polygon. | ||
* @example | ||
* var bbox = [-70, 40, -60, 60]; | ||
* var points = turf.random('points', 100, { bbox: bbox }); |
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.
The random
module has changed in the newest release.
var points = turf.randomPoints(100, { bbox: bbox });
@stebogit I guess we could re-implement the random()
method with the legacy behavior.
packages/turf-voronoi/index.js
Outdated
* var voronoiPolygons = turf.voronoi(points, bbox); | ||
*/ | ||
function voronoi(points, bbox) { | ||
if (!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.
Agreed, both points
& bbox
should return Errors if not provided.
if (!points) throw new Error('points is required');
if (!bbox) throw new Error('bbox is required');
packages/turf-voronoi/index.js
Outdated
if (!points) { | ||
return; | ||
} | ||
var features = Array.isArray(points) ? points : points.features; |
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.
Array of Features shouldn't be handled since it's not a valid GeoJSON input.
packages/turf-voronoi/index.js
Outdated
|
||
var polygonsd3 = d3v(pointsd3).polygons(); | ||
|
||
return { |
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.
👍
packages/turf-voronoi/package.json
Outdated
@@ -0,0 +1,51 @@ | |||
{ | |||
"name": "@turf/voronoi", | |||
"version": "0.1.0", |
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.
The version can be 5.0.0
since this will be published under that major release.
packages/turf-voronoi/package.json
Outdated
{ | ||
"name": "@turf/voronoi", | ||
"version": "0.1.0", | ||
"description": "turf module to generate Voronoi polygons from a set of points and a bounding box", |
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 are using JSDocs for the description of the modules, since no one will be maintaining the package.json
we just define this description as very generic.
"description": "turf voronoi module",
packages/turf-voronoi/package.json
Outdated
"module": "index", | ||
"jsnext:main": "index", | ||
"files": [ | ||
"index.js" |
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.
You'll also need to provide a Typescript definition, index.d.ts
and the main.js
(Rollup build output).
Look at any other TurfJS package.json
for examples.
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.
Ok, added - but I have no idea what I'm doing here, or how this file is used. This would be a good thing to document in CONTRIBUTING.md.
packages/turf-voronoi/package.json
Outdated
"dependencies": { | ||
"@turf/linestring-to-polygon": "^5.0.0", | ||
"d3-voronoi": "^1.1.2", | ||
"turf-helpers": "^3.x" |
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.
Use the namespace turf "@turf/helpers": "5.x"
Thanks very much for the comments and suggestions above. I think I implemented them all. Just a thought: an alternative way to express the body of this function is in functional style: return featureCollection(
d3voronoi.voronoi()
.extent([[bbox[0], bbox[1]], [bbox[2], bbox[3]]])(points.features.map(getCoords))
.polygons()
.map(coordsToPolygon)
); Any preferences one way or the other? |
@stevage I believe |
@stevage Functional style works, Edit: You're example did use return featureCollection(
d3voronoi.voronoi()
.extent([[bbox[0], bbox[1]], [bbox[2], bbox[3]]])(points.features.map(getCoords))
.polygons()
.map(coordsToPolygon)
); |
/** | ||
* http://turfjs.org/docs/#voronoi | ||
*/ | ||
export default function voronoi( |
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.
👍 Look great! Don't have to change a thing
These TurfJS typescript definitions are getting a lot easier to write as we go.
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.
👍 published under @turf/voronoi
❌ Error
Could be fixed on
You can also test this by not doing the Rollup build and only using
|
Oh...bugger. I was just making all these same changes at my end, heh. |
Lol 😄 I didn't do that much, mostly published the module, that was the biggest thing. |
Heh. Looks like there are still a lot of undocumented steps in the CONTRIBUTING.md, like adding the package to turf/index.js, turf/index.d.ts, turf/package.json etc. |
@stevage Well you can't add Right now the tests are pretty aggressive, they could be more Slack on the unpublished modules. @stevage Feel free to add to the CONTRIBUTING guide :) It's been mostly the same people contributing new modules. |
❌ Un-related error@stebogit This error keeps poping up a bunch of times, it comes from I wonder why this happens sporadically, maybe we could tone down the example for that module to prevent any un-related errors.
|
@stevage 16 commits later... All checks passed! 😀 I'm ready to merge whenever you're good with it. |
Woo :) Yeah, I'm good with it. Although I probably should have mentioned earlier that one downside of using D3 is that the calculations are completely 2D. I don't imagine it will materially matter for most applications, but close to the poles the polygons will be completely wrong. |
Totally not worried about the 3D stuff, barely any modules in turf handle that type of complexity. Oh one more thing, could you add Mike as a contributor to this module since he pretty much did the 95% of the work to make this happen :) |
You might consider using Philippe Rivière’s d3-geo-voronoi instead of d3-voronoi for geographic polygons, or Loren Petrich's spherical Delaunay Triangulation library, upon which d3-geo-voronoi is based. However, note that adopting a spherical Voronoi library introduces some subtle challenges when working with standard GeoJSON, since standard GeoJSON uses planar equirectangular coordinates rather than true spherical coordinates. (See the d3-geo README.) Also, there’s no need to add me as a contributor just because you’re using d3-voronoi, though thank you. The d3-voronoi library is based largely on work by Raymond Hill and Steven Fortune. If I ever find the time, I am planning on rewriting d3-voronoi to use Vladimir Agafonkin’s delaunator, since it will be faster and more stable for certain numerical edge cases. |
Wow, thanks for the feedback @mbostock! We can definitely switch over to Looking forward to the new and improved
We try to include dependency author's to the contributor list, as much as we can, it's the least we can do for including this awesome library 😄 |
I've tried to implement |
The thing with geo-voronoi is that it uses the correct (geodesic) distance, and when dealing with Voronoi you probably want distances to be meaningful. Also, the results of a geometric operation should be independent of the reference rotation. Please give an example of how d3-geo-voronoi "keeps breaking" and don't hesitate to open an issue as necessary on https://github.com/Fil/d3-geo-voronoi; I'll try and help if I can. It's true that this code is quite unstable. |
@Fil Thanks for the feedback, I'll send some examples to |
Just to follow-up on the For now the best results I get when I convert the WGS-84 points to mercator projection, apply planar voronoi and then convert the coordinates back to WGS-84. |
Can you share an actual example? thanks |
Hi @Fil, I am fan of your work, would love to see it working. Here is the data set I was testing it on: https://jsfiddle.net/h6qz279n/1/ The bugs look very similar to this issue. Let me know if I am doing something wrong. |
Leaflet doesn't support geodesic line interpolation, it just draws a straight line between the projected coordinates. In that sense it's not compatible with geoVoronoi which generates polygons that cover the whole sphere. In this case I'd use a planar voronoi—maybe add 4 “corners” around the city. Note that on a small area the error is negligible https://observablehq.com/@visionscarto/planar-vs-spherical-voronoi On top of that I think you are correct that this dataset also triggers numerical instability (Fil/d3-geo-voronoi#10). |
Yes, I implemented the planar voronoi (via I don't think I really understand the reason leaflet doesn't work with geoVoronoi. Are the resulting coordinates imprecise? I tried reading the README but could not crack how it should work either. Is it even possible to convert the results of geoVoronoi into a planar map like Leaflet? I thought that when I get GPS coordinates as a result, they would just work on the map... Also, do you then agree that it would be better to use |
They do! The points are at the correct location. What is different is line interpolation (the actual sequence of intermediary points that describes a line on the screen). Leaflet draws straight lines between points, meaning it expects the user to have done line interpolation in advance if they have complex paths, whereas D3 does line interpolation during projection.
You can convert spherical coordinates to equirectangular coordinates using d3.geoProject (from the d3-geo-projection module):
In terms of point projection, this is a no-op: a point like [longitude, latitude] is projected to the same values. However, this code computes the proper line interpolations, which you can then pass to leaflet. But on top of that problem, there is the additional issue of instability (Fil/d3-geo-voronoi#10). You can solve this currently, for this dataset, by adding a small perturbation to the coordinates:
but of course this is not fully satisfying.
I don't know. For local or regional data it seems more straightforward (and faster) to use d3-delaunay, but the larger the area, the more incorrect it gets, so for global data you need the spherical approach. |
Thanks for a thorough and clear explanation. Well, that sucks. 😂 I really wanted to have a one-size-fits-all solution, which is not the case here, unfortunately. For my use case, I will use the planar version, for For now, the algorithm skews the polygons (since it works with cartesian coordinates instead of WGS-84) and even places the polygons outside the points, so basically anything would be better than the current version. What do you think? |
I'm not sure why polygons would be "outside the points", an explicit example would help to understand. In my understanding, skewing happens because the local plane expressed in degrees is not isotropic: a distance of 10 km to the East or West doesn't correspond to the same number of degrees as the same distance to the North or South (unless you are near the equator). (In other words, a circle is mapped to an ellipse.) This is not tied to d3.voronoi, it will be the same when you replace d3.voronoi by d3.delaunay. Your approach "convert the WGS-84 points to mercator projection, apply planar voronoi and then convert the coordinates back to WGS-84" matches the Leaflet projection, so it sounds to me like the best approach if you want to make polygons "look" correct. For local data, the approximation will be very good, even though not optimal (using a projection tangent to the local plane would be even better… but it would involve a determination of the local plane). This amounts to what we do in Observable Plot with the voronoi mark, where we compute it on the projected points; except here, you need to go back to WGS-84 because you're passing the polygons to Leaflet, which will then project them (again) with Mercator 😀. I'd still recommend to move from d3.voronoi to d3.delaunay, because performance and precision are orders of magnitude better. A problem you will have is if you want to draw a voronoi of points that are close to the antemeridian. In that case you'll need to add a rotation in your intermediate mercator. If we could fix Fil/d3-geo-voronoi#10 I'd recommend the more complex but exact approach I outlined above (geoVoronoi + geoProjection to WSG-84 for line interpolation). |
I meant, when I tried applying plain |
npm test
at the sub modules where changes have occurred.npm run lint
to ensure code style at the turf module level.npm run lint
fails with this helpful message:I take it I need to include some invalid jsdoc to pass the lint?
Submitting a new TurfJS Module.
./scripts/generate-readmes
to createREADME.md
.package.json
using "Full Name <@github Username>".