-
Notifications
You must be signed in to change notification settings - Fork 27
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
Geodesic Convolution Buffers #33
base: master
Are you sure you want to change the base?
Conversation
…rns polygon, multifeatures implemented. All seems to work as intended
I just notices I should have better submitted by pull-request to the artisinal branch. Can I undo this? If you can (and agree), please do! |
// if(Array.prototype.equals) console.warn("Overriding existing Array.prototype.equals. Possible causes: New API defines the method, there's a framework conflict or you've got double inclusions in your code."); | ||
// attach the .equals method to Array's prototype to call it on any array | ||
Array.prototype.equals = function (array) { |
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.
Per the JavaScript norms currently, extending built-in objects can be dangerous, since the extensions may break other code or native behavior. Could this be implemented instead as a function that takes two arrays, and the same with the mod
extension?
…implementing the approach of Polygon Offsetting By Computing Winding Numbers paper and fixing the creation of false inner rings
Thanks for the feedback on my previous comment! As you can see, I’ve continued to work on this turf-buffer PR and on its dependencies simplepolygon and geojson-polygon-self-intersections - albeit bit by bit, since this is spare time work for now (and it's lovely summertime outside and stuff). I tested this implementation against all fixtures in ‘tile-cover’, as suggested by @morganherlocker. I found three minor issues, and was able to fix all three of them quickly. Now all 179 features from this test-set produce a correct buffer (I visually checked all of them). And look: the buffer radius nicely scales with latitude when projected in webmercator: :) Next, I implemented a spatial index in simplepolygon’s two double for-loops, which are now one for-loop with one tree search. The resulting new benchmark measurements give:
I’m very happy with these speed gains here (and loving rbush). There's no clear slowest step in turf-buffer or simplepolygon anymore. Next (and probably final) attempt to gain speed will be to turf-simplify large features. Comming up! Ideas/opinions welcome! |
Here we are! I implemented line and polygon simplification. Super small edit, then quite some testing and debugging. Remarkably, although turf-simplify doesn't guarantee to preserve topology when polygons have holes (and breaks it in some of the test cases), this doesn't prevent the calculation of a correct buffer. :) What's the effect, you ask... PerformanceI set the tolerance of turf-simplify to be the In other cases, such as for features with very few coordinates (which can't be simplified much more) or small buffer radii compared to the inter-coordinate-distance (such as in the tests I did earlier), there is quasi no speed gain. Therefore, the previous metrics still apply. Known issues
Furthermore, this commit fixes the second category of issues I mentioned in the first post. And now...?This pull request kindly yet impatiently awaits further testing, comments and/or approval. @tmcw @mourner @morganherlocker @tcql or anyone else: let's here it from you! |
Wow, amazing work!
Are you saying this implementation fails when it receives non-simple features, or that it sometimes outputs non-simple features? |
I mean that this implementation fails on some input features that aren't conform the Simple Feature standard. I talked a bit about some cases in my first comment on this PR, and just wanted to mention it again here for completeness. |
Hey @mclaeysb, great meeting you in Brussels. Super interesting work here. Are the known issues listed above something you're working on? What else does this branch need? |
Hey @aaronlidman, thanks for checking this out! I'm not currently working on these issues, since most of them are OK:
In my opinion, this branch mostly needs:
Also, this is my first JS project and my first PR, so any comments on my code, or tips & helpful tools, are much appreciated! In the meantime, I found out about @tmcw's mom's cookies and am hoping this is somewhat eligible...? |
@DenisCarriere But more than knowing the time spent at each successive line in this (asynchronous) code, it'd be good to know, say, the total (summed) time spent in the many |
@mclaeysb I think what you're looking for goes beyond the scope of |
@mclaeysb v8 has some decent profiling flags for this type of info that you can access from the exampleSay we have a benchmark we want to run called node --prof bench.js This will run your benchmark in a special v8 debug mode that samples the v8 process at a set interval (default 1 ms), and log some info about what the process was working on each time it was sampled. You can set additional flags to log more or less data (ie: garbage collection events, memory usage, etc.). Once the process exits, you will see a
Node has another flag that allows for v8 to parse and aggregate these dumps, # note that the name of the log is generated on the fly;
# change the name based on your actual log file
node --prof-process v8.log This will output a helpful summary of how much time your process spent on each function, split out by naitive, C++ functions and JavaScript functions.
I usually start at the top of this list until every function is as fast as I can possibly make it. Functions with an PS: There are many ways to visualize these logs -- some built into v8 and some not. Here's some samples I often rely on, but generating them is probably more appropriate for an extended blog post. 😄 --log_timer_events with v8's profviz tool flamegraph visualizing callstack depth |
Another approach which should be simpler and more convenient in this case is running Chrome DevTools profiler in the browser (preferably on a very big polygon to get statistically significant data). |
I am so looking for this, I noticed Turf was sending out inaccurate Geodesic buffers so I dropped it. |
I notice it has been since September since the last comment, is there something holding this up? Would I be able to grab the non-Minified version and replace the buffercode with the code in index.js Edit: This doesn't look Easy to do after looking at the non-minified code. I could try to figure out how to build this on windows with these changes I guess. Any shortcuts on how I can research making a build with your changes? |
Hey @flipper44. Thanks for your intrest in this pull! I don't understand you first question/remark regarding the 'center segment' etc. Could you try to explain it again? Regarding your other remarks, there points might help you out:
Cheers! |
Circle vs Buffer Above |
Hi @flipper44 I'll answer your points here:
Regards! |
In terms of speed, JSTS isn't using Haversine calculations and distances, there is no way that A geodesic buffer can even come close in speed to a planar buffer in speed. And the Current buffer only seems to do Haversine North and south and nothing in between, When I look at the sheer area of error I still am at a loss for why More GIS people aren't also pushing for this. Perhaps It would make sense to have three functions, one called PlanarBuffer which would be best for local coordinates systems, One called FastGeodesicBuffer, and one called TrueGeodesicBuffer (yours) |
Got past the warnings on npm install is there a javascript library that is missing? |
General rule of thumb: if you see an error, specify it. What is the error? What is the exact text of the error message? |
I was missing the browserify command
…On Feb 16, 2017 3:06 PM, "Tom MacWright" ***@***.***> wrote:
When I try to load the script the requires error out.
General rule of thumb: if you see an error, specify it. What is the error?
What is the exact text of the error message?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/APQv9_em9SEsqJcJhFEoExQip9xODf3Qks5rdLpbgaJpZM4Iz2pa>
.
|
An update: Turf has been modularised, and |
I'm very interested in seeing a geodetic buffer in turf. Did this discussion die off or move somewhere else? |
I had some free time and dove into the sea of buffer ellipses, took on the winding number snake and sword-fought with edge-intersections. It was hard, but I endured, and today I can finally present to you the result of my quest:
This pull implements geodetic buffers for GeoJSON Points, LineSegments and Polygons (possibly having inner loops) and their Multi- counterparts, ... without JSTS dependency!
I included a basic test, but please test your own files and let me know if you encounter errors or problems.
Here's how I went about it:
Known issues
How geodesic is this approach?
I really like geodesic-ness (I hope that's a word) discussions, and I hope to see more of them in Turf / live / ... discussions (since the Turf approach to not "project it + do a euclidean algorithm" but instead "do it on a sphere directly" is pretty fundamental to it) (Also also ref many discussion in turf-square-grid etc.). Let's talk!
Further development
Hopefully soon...
Citations
[1] Subramaniam, Lavanya. Partition of a Non-simple Polygon Into Simple Polygons. Diss. University of South Alabama, 2003
[2] Chen, Xiaorui, and Sara McMains. "Polygon offsetting by computing winding numbers." ASME 2005 International Design Engineering Technical Conferences and Computers and Information in Engineering Conference. American Society of Mechanical Engineers, 2005.