Sg/update intersects #22

skygering · 2023-10-04T19:40:49Z

I updated and tested the intersects.jl file and also some other files that were connected got a few touch ups.

The details of the implementation are detailed at the top of intersects.jl.

Something I am not sure about is the use of the meets keyword in the intersects functions. I get how it works for a line segment (just two points) in that it can exclude endpoints. However, what does that mean for a line string (multiple segments) with a line through the segment that happens to pass through a vertex. Right now, meets = 0 means that that isn't tagged as intersecting, when in my mind it is. However, this is a degenerate case. I guess I am not totally sure of the use cases for meets. Thoughts?

I also changed a test case for the same polygon overlaping with itself. This was false before, but I think it should be true as they are completely overlapping.

skygering · 2023-10-04T19:42:51Z

test/methods/bools.jl

@@ -128,7 +128,7 @@ import GeometryOps as GO
        (-53.34136962890625, 28.430052892335723),
        (-53.57208251953125, 28.287451910503744),
    ]])
-	@test GO.overlaps(pl3, pl4) == false
+	@test GO.overlaps(pl3, pl4) == true # this was false before... why?


This is the test that I changed!

~~No idea why it was wrong, I copied these from Turf.jl.~~

Actually it doesn't overlap - overlaps cant be equal:

| a overlaps b: they have some but not all points in common, they have the same dimension, and the intersection of the interiors of the two geometries has the same dimension as the geometries themselves.

https://en.wikipedia.org/wiki/DE-9IM

I guess testing these against LibGEOS is best?

(This was nothing like a finished state, I just pasted in the Turf.jl tests to have something quick to code and bugfix against. Thanks for adding so many more tests!!)

That makes sense! They were using intersects as equivalent to overlaps, which isn't valid I think since two of the same geometries on top of one another would have an intersection, namely the whole geometry. I will double check with LibGEOs though.

rafaqz · 2023-10-04T20:39:08Z

Do we need meets? LibGEOS doesn't have that.

You can do touches(a, b) to check meets?

(likely meets was just in some code already and never got deleted - I think it may have been more of an internal use thing - feel free to carve this code up however is necessary)

skygering · 2023-10-18T18:32:39Z

@rafaqz Any idea why this is failing on 1.6 but passing in 1.0? It is also passing on 1.9 for me. It is failing on the call to findfirst on line 133 in equals.jl due to a lack of key method definition. Is that a 1.6 problem? I can replace findfirst with a for loop... but that seems unneeded.

I also ended up updating equals and overlaps since my changes to intersects made some overlaps tests fail and then I needed equals in overlaps.

I do have a few questions. I will add comments in those locations. I hope we can get this merged!

skygering · 2023-10-18T18:34:57Z

src/methods/bools.jl

@@ -193,6 +171,26 @@ function point_on_line(point, line; ignore_end_vertices::Bool=false)::Bool
    return false
 end

+function point_on_seg(point, start, stop)


I added this because the point_on_segment function was complicated with all of the potential endpoint options. Do we want all of those options? I removed the similar option meets as we discussed since it wasn't something that is available in LibGEOS. I can see how this might be helpful in other functions though...

skygering · 2023-10-18T18:37:01Z

src/methods/equals.jl

+    # Check line lengths match
+    n1 = GI.npoint(l1)
+    n2 = GI.npoint(l2)
+    # TODO: do we need to account for repeated last point??


If there are two polygons, one with a repeated first point at the end, and one without the repeated point, should they still be equivalent?

Thoughts on this @rafaqz or @asinghvi17? It isn't something I can compare with LibGEOS since LibGEOS won't allow you to create a polygon without closing it. However, GeoInterface will. By definition, a Polygon must be closed, so the repeated last point is just a convention. However, this is also true for linear rings, but not line strings, which aren't by convention closed. However, linestring and linear rings can be equal and in that case, I think that the linestring would need to have the repeated last point to be equivalent to a linear ring. This seems like it could get confusing.

I updated this! It now works. Hopefully, my implementation isn't too complicated. I will push soon.

Sorry forgot to reply. Its harder for GeoInterface because GeometryBasics uses Linestring for everything so we can enforce these things.

Glad you found a solution

skygering · 2023-10-18T18:39:25Z

test/methods/equals.jl

+    # Different rings
+    @test !GO.equals(r1, r2) && !GO.equals(r2, r1)
+    # Equal linear ring and line string
+    @test !GO.equals(r2, l3) # TODO: should these be equal?


If we have two curves, but one is a linear ring and one is a line string , but they have all the same points, should they be the same? I don't think so, but maybe...

I have determined that these should be equal given LibGEOS outputs. Similarly, a multi-polygon that holds a single polygon is equal to that same polygon. So it is about what the shape contains, not about the shape type, although obviously, some types preclude equality. So curves of all types can be equal and types and their "multitypes" can be equal if the multitype just holds one geometry.

skygering · 2023-10-18T18:40:09Z

test/methods/overlaps.jl

+    # Test one polygon within the other
+    @test GO.overlaps(p2, p4) == GO.overlaps(p4, p2) == LG.overlaps(p2, p4)
+
+    # @test_throws MethodError GO.overlaps(pl1, (1, 1))  # I think these should be false


Is there something wrong with allowing this? This works for my code right now... should I find a way to make it throw an error?

Can a point overlap a popygon? The dimensions dont match

https://en.m.wikipedia.org/wiki/DE-9IM

I guess it should be false rather than error, true

I guess I was wondering more from a perspective of this is passing in a tuple, rather than a Point. And if that is allowed. Mine do return false due to the difference in dimension.

Oh right.

If a method here works for Point it should work for all p where GI.trait(p) isa GI.PointTrait

rafaqz · 2023-10-18T19:12:13Z

The passsing tests are on 1.9 too.

Looks like somwhere a Generator isnt being collected and that fails on 1.6 because keys isnt defined for ut. Maybe in equals or overlaps ? getgeom returns a Generator, it could be related to that...

skygering · 2023-10-18T20:28:54Z

Okay I just removed the call to findfirst and basically just wrote the code for findfirst. At least the tests all pass now.

skygering · 2023-10-26T14:50:11Z

Is everyone okay with me merging?

rafaqz · 2023-10-26T15:05:27Z

Thanks!! code looks great.

skygering and others added 5 commits October 2, 2023 16:55

Fix up intersection point base calculation

aba090f

Update intersects and add line tests

ab16785

Add more tests and debug intersects

7e75e86

Add comments to point_in_poly

a7a7367

Remove CairoMakie

b99e37d

skygering commented Oct 4, 2023

View reviewed changes

Update equals and overlaps

319bd88

skygering commented Oct 18, 2023

View reviewed changes

Remove use of findfirst for 1.6 compat

0b9799d

skygering added 4 commits October 20, 2023 00:27

Updated geom, multi-geom equality

90fff1c

Merge branch 'asinghvi17:main' into sg/update_intersects

ab211f3

Add comments and tests

41eb3a1

Remove trial file

9b64574

skygering merged commit 8851c23 into JuliaGeo:main Oct 26, 2023
3 checks passed

skygering deleted the sg/update_intersects branch October 26, 2023 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sg/update intersects #22

Sg/update intersects #22

skygering commented Oct 4, 2023

skygering Oct 4, 2023

rafaqz Oct 4, 2023 •

edited

Loading

skygering Oct 4, 2023

rafaqz commented Oct 4, 2023 •

edited

Loading

skygering commented Oct 18, 2023

skygering Oct 18, 2023

skygering Oct 18, 2023

skygering Oct 21, 2023 •

edited

Loading

skygering Oct 25, 2023

rafaqz Oct 26, 2023 •

edited

Loading

skygering Oct 18, 2023

skygering Oct 19, 2023 •

edited

Loading

skygering Oct 18, 2023

rafaqz Oct 18, 2023 •

edited

Loading

skygering Oct 18, 2023

rafaqz Oct 18, 2023

rafaqz commented Oct 18, 2023

skygering commented Oct 18, 2023

skygering commented Oct 26, 2023

rafaqz commented Oct 26, 2023

Sg/update intersects #22

Sg/update intersects #22

Conversation

skygering commented Oct 4, 2023

Choose a reason for hiding this comment

rafaqz Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz commented Oct 4, 2023 • edited Loading

skygering commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skygering Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skygering Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz commented Oct 18, 2023

skygering commented Oct 18, 2023

skygering commented Oct 26, 2023

rafaqz commented Oct 26, 2023

rafaqz Oct 4, 2023 •

edited

Loading

rafaqz commented Oct 4, 2023 •

edited

Loading

skygering Oct 21, 2023 •

edited

Loading

rafaqz Oct 26, 2023 •

edited

Loading

skygering Oct 19, 2023 •

edited

Loading

rafaqz Oct 18, 2023 •

edited

Loading