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

Fix polygonize #141

Merged
merged 35 commits into from
May 16, 2024
Merged

Fix polygonize #141

merged 35 commits into from
May 16, 2024

Conversation

rafaqz
Copy link
Member

@rafaqz rafaqz commented May 13, 2024

I forgot to PR this months ago.

But it mostly rewites polygonize and uses the uglier but more practically useful exact pixel approach rather than having any angles or curves in the output. It also fixes some bugs and lets you define the contour cutoff yourself.

@skygering I added a performance optimization on coveredby, because it a huge gain for polygonize.

# with extent check in `coveredby`
julia> @btime polygonize(>(0.6), rand(100, 100), minpoints=3);
  2.985 ms (5774 allocations: 2.92 MiB)

# without it
julia> @btime polygonize(>(0.6), rand(100, 100), minpoints=3);
  15.703 ms (5625 allocations: 2.93 MiB)

I'm not sure if that's the way to do it, it should probably go into _line_polygon_process so its used for everything. But, there are cases where not intersecting should return true and others where it should return false depending on the other arguments?

Extent checks are done generically for all predicates now

Oh and this needs tests!!

@rafaqz rafaqz marked this pull request as ready for review May 13, 2024 16:32
@rafaqz rafaqz requested a review from skygering May 13, 2024 16:32
src/methods/polygonize.jl Outdated Show resolved Hide resolved
src/methods/polygonize.jl Outdated Show resolved Hide resolved
test/methods/bools.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@skygering skygering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting optimizations. I don't think the logic is quite there but let me know what you think about my comments.

@asinghvi17
Copy link
Member

asinghvi17 commented May 15, 2024

We should propagate GI.crs(A) to the created geometries, that gets this PR 50% of the way there to full Raster support (the other 50% would probably be an extension on DimensionalData.jl to support dimensional axes).

Edit: implemented in #143

asinghvi17 and others added 11 commits May 16, 2024 08:26
With the changes from the last commit, even Rasters work with roundtripped CRS.  The only thing remaining to be done is to figure out how to generate edge vectors for interval sampling, which would probably be an extension on Rasters in GeometryOps, since we're overriding an internal GO method.
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
@rafaqz rafaqz merged commit 0321d26 into main May 16, 2024
4 checks passed
@rafaqz rafaqz deleted the fix_polygonize branch May 16, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants