Skip to content
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

isobands fails on flat data #1797

Closed
Eh2406 opened this issue Nov 25, 2019 · 5 comments · Fixed by #2527
Closed

isobands fails on flat data #1797

Eh2406 opened this issue Nov 25, 2019 · 5 comments · Fixed by #2527

Comments

@Eh2406
Copy link

Eh2406 commented Nov 25, 2019

We are hitting this in a map @superchad and I are developing. When all the points in the grid fall in the same band then isobands returns no geometry instead of the bbox of the grid. If one point on the edge has a different band than the hole area is filled in correctly. Instead of a geojson I have a failing test v7...Eh2406:v7

@stebogit
Copy link
Collaborator

stebogit commented Nov 30, 2019

You're right, the output in your case should be a MultiPolygon with the entire grid area as single band in the correct break level. The current implementation returns instead an empty MultiPolygon (which might be an invalid GeoJSON).

@stebogit stebogit added the bug label Nov 30, 2019
@Eh2406
Copy link
Author

Eh2406 commented Dec 2, 2019

Do you have advice for how you would like to see this fixed? Are you open to merging PRs? (I assume to the v7 branch, but if that is not where you want them say so) How much of a rewrite/cleanup is exemptible? (for example the BandGrid2Areas looks unused and does not work with my testing, would it help for me to just remove it?)

I have spent the past week trying to dig in. It looks like the boundary edges are added in the loop in BandGrid2AreaPaths, but that is only triggered when an internal edge hits the boundary. I think we can separate the core of that loop into a function, and then call the function on (0, 0) if it should be in the band. But I am very open to advice.

@stebogit
Copy link
Collaborator

stebogit commented Dec 3, 2019

@Eh2406 I originally used MarchingSquares.js importing the library via mpn, then, on an unsuccessful attempt to convert Turf to TypeScript, the library was included as local dependency.
For any issue/question on the library you should try to contact Ronny Lorenz, he has been very helpful in the past when I developed the Turf module.
I know he worked on few fixes and improvements of its MS algorithm after I implemented @turf/isobands, but I'm not sure we ever included those changes back in. You might want to try if the latest version does fix your issue.

@Eh2406
Copy link
Author

Eh2406 commented Dec 3, 2019

You are reading our minds! I was just going to comment that it looked like you had copied the code from there and that the npm version does not have this bug. Given that the v7 branch is giving up on TypeScript, do you want a PR moving back to using the npm version?

@stebogit
Copy link
Collaborator

stebogit commented Dec 3, 2019

Given that the v7 branch is giving up on TypeScript, do you want a PR moving back to using the npm version?

👍

smallsaucepan added a commit to smallsaucepan/turf that referenced this issue Nov 1, 2023
…s library from NPM. This commit supercedes PR Turfjs#1801 which is a few years old now. Resolves Turfjs#1797 based on a test case mentioned in that issue (added to unit tests in isobands). Slight changes to resulting coordinates in most fixtures, though all fixtures verified visually using geojson.io
smallsaucepan added a commit to smallsaucepan/turf that referenced this issue Nov 3, 2023
…s library from NPM. This commit supercedes PR Turfjs#1801 which is a few years old now. Resolves Turfjs#1797 based on a test case mentioned in that issue (added to unit tests in isobands). Slight changes to resulting coordinates in most fixtures, though all fixtures verified visually using geojson.io
smallsaucepan added a commit to smallsaucepan/turf that referenced this issue Nov 9, 2023
…s library from NPM. This commit supercedes PR Turfjs#1801 which is a few years old now. Resolves Turfjs#1797 based on a test case mentioned in that issue (added to unit tests in isobands). Slight changes to resulting coordinates in most fixtures, though all fixtures verified visually using geojson.io
smallsaucepan added a commit that referenced this issue Nov 9, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…st break" bug (#2527)

* Migrating isobands and isolines to use the third party marchingsquares library from NPM. This commit supercedes PR #1801 which is a few years old now. Resolves #1797 based on a test case mentioned in that issue (added to unit tests in isobands). Slight changes to resulting coordinates in most fixtures, though all fixtures verified visually using geojson.io

* Fixing turf-isolines to no longer ignore the first break passed. Where it looked like the unit tests have had a 0 break added as a work around e.g. 5 breaks but only 4 break properties, have removed the presumed workaround and reviewed test fixture output. Resolves #2129.

* Rolling back cloning workaround from commit d7c5322. This appears to have been unnecessary, possibly already having been fixed by adding linearRing: false to the isoContours call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants