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.

Sign up for GitHub

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

actually test embed_extent #29

Merged
merged 4 commits into from
Dec 24, 2023
Merged

actually test embed_extent #29

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26d47c8
actually test embed_extent
rafaqz Dec 22, 2023
7a572f6
fix Extents dep
rafaqz Dec 23, 2023
ae8ce95
document transformation keywords
rafaqz Dec 23, 2023
318bfd6
bugfix keyword formatting
rafaqz Dec 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/GeometryOps.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ include("methods/polygonize.jl")
include("methods/barycentric.jl")
include("methods/equals.jl")

include("transformations/extent.jl")
include("transformations/flip.jl")
include("transformations/simplify.jl")
include("transformations/reproject.jl")
Expand Down
19 changes: 12 additions & 7 deletions src/transformations/extent.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,24 @@ calculating and adding an `Extents.Extent` to all objects.

This can improve performance when extents need to be checked multiple times.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could use a bit more comments. I wasn't familiar with this concept and it took a good few minutes to understand what was going on. Maybe a brief description of what extents are and how to access them after running.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh right, I'm assuming a lot of knowledge here. Definately more docs could help!

The ideas is you can use the extent as e.g. a fast pointinpolygon proxy. If its in the extent you need to actually check the polygon, if its not you know in a few ns.

"""
embed_extent(x; kw...) = apply(AbstractTrait, x; kw...)
embed_extent(x; kw...) = apply(extent_applicator, GI.AbstractTrait, x; kw...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also might want to specify here what keywords are possible. Cross-referencing with the with primitives.jl, I can see that it they are: crs, calc_extent, and threaded... however, down below we recalculate crs and we are obviously calculating extent so we might what to specify why we use apply.

Copy link
Member Author

@rafaqz rafaqz Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh probably this can all be written just using one line - apply(identity, PointTrait, x; calc_extent=true) !!!

calc_extent already preopagates the extents efficiently.

And then the threaded and crs keywords make sense. We can just block the calc_extent keyword.

lol


# We recursively run `embed_extent` so the lowest level
# extent is calculated first and bubbles back up.
# This means we touch each point only once.
extent_applicator(x) = extent_applicator(trait(x), x)
extent_applicator(::Nothing, xs::AbstractArray) = embed_extent.(xs)
extent_applicator(::Union{AbstractCurveTrait,MultiPointTrait}, point) = point

function extent_applicator(trait::AbstractGeometryTrait, geom)
function extent_applicator(trait::GI.AbstractGeometryTrait, geom)
children_with_extents = map(GI.getgeom(geom)) do g
embed_extent(g)
end
wrapper_type = GI.geointerface_geomtype(trait)
extent = GI.extent(wrapper_type(children_with_extents))
return wrapper_type(children_with_extents, extent)
return wrapper_type(children_with_extents; extent, crs=GI.crs(geom))
end
extent_applicator(::PointTrait, point) = point
extent_applicator(::PointTrait, point) = point
function extent_applicator(trait::Union{GI.AbstractCurveTrait,GI.MultiPointTrait}, geom)
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
wrapper_type = GI.geointerface_geomtype(trait)
extent = GI.extent(geom)
return wrapper_type(geom; extent, crs=GI.crs(geom))
end
extent_applicator(::GI.PointTrait, point) = point
1 change: 1 addition & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const GO = GeometryOps
@testset "Signed Area" begin include("methods/signed_area.jl") end
@testset "Overlaps" begin include("methods/overlaps.jl") end
# Transformations
@testset "Embed Extent" begin include("transformations/extent.jl") end
@testset "Reproject" begin include("transformations/reproject.jl") end
@testset "Flip" begin include("transformations/flip.jl") end
@testset "Simplify" begin include("transformations/simplify.jl") end
Expand Down
17 changes: 17 additions & 0 deletions test/transformations/extent.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
using Test

import GeoInterface as GI
import GeometryOps as GO
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to test at least one curve or something that isn't nested since that is a separate function

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use apply for everything than it also needs less tests.

import Extents
using GeoInterface.Wrappers

@testset "embed_extent" begin
poly = GI.Polygon([GI.LinearRing([(1, 2), (3, 4), (5, 6), (1, 2)]),
GI.LinearRing([(3, 4), (5, 6), (6, 7), (3, 4)])])

ext_poly = GO.embed_extent(poly)
lr1, lr2 = GI.getgeom(ext_poly)
@test ext_poly.extent == Extents.Extent(X=(1, 6), Y=(2, 7))
@test lr1.extent == Extents.Extent(X=(1, 5), Y=(2, 6))
@test lr2.extent == Extents.Extent(X=(3, 6), Y=(4, 7))
end
Loading