-
Notifications
You must be signed in to change notification settings - Fork 990
Rewrite @turf/isolines #2918
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
Rewrite @turf/isolines #2918
Conversation
cdcfaec to
b3b9799
Compare
|
Brilliant work @mfedderly! When this first came up I made a start on a rewrite package. What do you think about externalising this implementation through there? The author of the marching-squares package on npm agreed to hand over that name to allow for a modern update. |
|
I think I prefer just putting things in Turf directly if we're going to be maintaining it. Over the years we've had issues consuming other people's packages due to various repo issues, like types not being correct, or nuance about package.json export fields, etc. We spent a pretty good amount of time on the monorepo infrastructure in this repo so it makes sense to try to use it where possible. The only place where I've seen this reasoning break down is when we've got code that we want to share between two Turf packages where we don't want the underlying code to itself be a public module, that gets a little tougher. |
|
If it's general purpose, self contained functionality though why not release it separately so other people can benefit? There's clearly a need for a non-GPL marching squares library. All those reasons about types and package.json no longer apply as we have control over packaging and publishing. Seems like win-win to me. |
b3b9799 to
6d5bf3f
Compare
|
I tried to make this faster for a few days, including some really neat ideas from d3-contour but was unable actually make the line segment reassembly phase faster. I'm going to leave it for now. It did give me some guidance on how to do the isobands work at least. |
|
@smallsaucepan my primary issue with making another repo is that then we have to maintain all of this stuff in another repo, where we already have all the infrastructure in Turf to add another package. I'd be more inclined to add a new @turf/marchingsquares than an entirely separate repo somewhere. I'm also not entirely convinced that marchingsquares code would be truly reusable. You're going to wind up with data format issues (some data will use |
90741d9 to
55a8a66
Compare
|
Ok I think I have a working isobands, but it is remarkably slow so I need to iterate some more. I'm just happy this thing is working at this point 😩 . I really struggled with the polygon reassembly logic. This is also 'cheating' in that it doesn't use the isobands logic from Wikipedia, but it instead just calculates the rings at each threshold and then does polygon difference. From looking at the geojson test output before and after... I may have actually fixed some bugs along the way too. 🤔 |
060359d to
1e75a17
Compare
| return results; | ||
| } | ||
|
|
||
| function isoContours( |
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.
@smallsaucepan After getting this all fixed up, this is probably the most shareable nugget of code, but I'm still not convinced that it will be broadly useful outside of Turf.
Specifically: because we're scaling latitudes, our y axis is essentially reversed from the index into matrix, that means that the tr/br/bl/tl lookups are logically reversed on the Y axis, and then the counterclockwise winding of the polygon is also logically reversed for the same reason.
I'm thinking that perhaps a well commented implementation to accompany the Wikipedia explanation would be more useful to someone who wants to do isolines/isobands on data that isn't already geojson. The Isobands polygon-ification step was really hard to reason about so I tried to add a lot of comments there.
As I'm looking at this PR, I kind of think that I like Turf having its own implementations that can serve as references to others if you don't want exactly what we're providing, but want to write your own code for something similar. We can also help control our own destiny a bit more (and avoid a polyclip-ts situation).
1e75a17 to
f796cd9
Compare
|
I rebased this to squash it into two commits. I think we should merge @turf/isolines and @turf/isobands changes in separate PRs, but because there's some shared code I've left them together at the moment just to help me clean up both changes at the same time. |
|
The strange output of @turf/isobands in matrix2 is fixed over here: #2925 |
f796cd9 to
fa844cf
Compare
smallsaucepan
left a comment
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.
Impressive work @mfedderly 👏
| "contributors": [ | ||
| "Stefano Borghi <@stebogit>" | ||
| "Stefano Borghi <@stebogit>", | ||
| "Matt Fedderly <@mfedderly>" |
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 you've added yourself to isobands twice. I'll add you to isolines package.json too 😉
The npm package marchingsquares has a complicated licensing story. I'm going to attempt to rewrite it from scratch and by using other compatible licensed code for reference.
@turf/isolines
Performance seems quite a bit faster (3x?) without much effort in optimization, timings taken on an M4 Pro Macbook Pro.
Rich diffs also seem to not be enabled on these geojson fixtures, so I added some screenshots using geojson.io below. To my eye they are the same, except that some of the array items have been rotated and/or the order of the linestrings may have changed.