Skip to content

MB-66210: Point and MultiPoint changes#18

Merged
Likith101 merged 3 commits into
masterfrom
points
May 6, 2025
Merged

MB-66210: Point and MultiPoint changes#18
Likith101 merged 3 commits into
masterfrom
points

Conversation

@Likith101

@Likith101 Likith101 commented Apr 9, 2025

Copy link
Copy Markdown
Member
  • Switched out all cell operations to point operations
  • All point comparisons will use approx equal which has an epsilon of 1e-15
  • LineStrings and MultiLineStrings will project point onto themselves and compare the distance between points for intersection
  • Polygons and MultiPolygons use shape indexes with a Closed Vertex Model which means the boundaries and the vertices are considered part of the shape
  • Circle will use distance from center to identify intersecting points
  • Envelopes will use bound checks to identify intersects
  • New Circles and Envelopes will now init themselves before returning
  • Removed a couple of unused functions
  • Added appropriate test cases

Note:
Projection onto a segment calculation might not be completely accurate. Projection of point (1,1) onto the line (-2,1) and (2,1) returns a point that has a distance of 50m from (1,1). Unable to verify if this distance is actually accurate or not.
Converting the point to a cell and then doing an intersects with the linestring still gives a a false which leads me to believe that the distance it is giving is actually correct.

@abhinavdangeti

Copy link
Copy Markdown
Member

@Likith101 it seems there're a few tests failing within s2/, although none that you've added it seems ..

ok  	github.com/blevesearch/geo/geojson	0.201s
ok  	github.com/blevesearch/geo/r1	0.827s
ok  	github.com/blevesearch/geo/r2	0.366s
ok  	github.com/blevesearch/geo/r3	0.520s
ok  	github.com/blevesearch/geo/s1	0.673s
--- FAIL: TestClosestEdgeQueryTrueDistanceLessThanChordAngleDistance (0.00s)
    edge_query_closest_test.go:122: CompareDistance((0.785167625848291916845767, -0.502004006908459698976799, -0.362634494177826782745910), (0.785630117324294330316548, -0.501876559404935029817807, -0.361808288839380542967206), 9.127564928066267e-07) = 1, want >= 0
--- FAIL: TestDet (0.00s)
    matrix3x3_test.go:355: [ 1.7400 2.7183 42.0000 ] [ 3.1416 1.4142 2.3026 ] [ 3.0000 1.2720 9.8976 ].det() = -56.83852522412309, want -56.838525224123096
--- FAIL: TestPointMeasuresPointArea (0.00s)
    point_measures_test.go:65: PointArea((0.257000000000000006217249, -0.572300000000000030908609, 0.112000000000000002331468), (0.257000000000000006217249, -0.572300000000000030908609, 0.112000000000000002331468), (0.257000000000000006217249, -0.572300000000000030908609, 0.112000000000000002331468)), got 1.9819499942010192e-34 want 0
--- FAIL: TestPredicatesRobustSignEqualities (0.00s)
    predicates_test.go:130: Testing equality for RobustSign. (0.577350269189625731058868, 0.577350269189625731058868, 0.577350269189625731058868) = (0.577350269189625842081171, 0.577350269189625842081171, 0.577350269189625842081171), got false want true
FAIL
FAIL	github.com/blevesearch/geo/s2	3.985s

It'd be good if you can address those alongside the github workflow you're about to add here. We can get that merged in first and rebase your sequence of commits over it - so commit validation is run over every pull request.

@abhinavdangeti

Copy link
Copy Markdown
Member

@Likith101 I've fixed the imports and added github actions for a tests workflow with #23 . Let's address the tests failures that I mentioned above on that commit separately, get that in and then move ahead with your commits.

@abhinavdangeti

Copy link
Copy Markdown
Member

@Likith101 I've upgraded and addressed the imports of our golang/geo fork with #24. Let's get this in first and rebase your commits over it.

@abhinavdangeti

Copy link
Copy Markdown
Member

Rebase time :)

 - Switched out all cell operations to point operations
 - All point comparisions will use approx equal which has an epsilon of 1e-15
 - LineStrings and MultiLineStrings will project point onto themselves
and compare the distance between points for intersection
 - Polygons and MultiPolygons use shape indexes with a Closed Vertex Model
which means the boundaries and the vertices are considered part of the shape
 - Circle will use distance from center to identify intersecting points
 - Envelopes will use bound checks to identify intersects
 - New Circles and Envelopes will now init themselves before returning
 - Added appropriate test cases

Note:
    Projection onto a segment calculation might not be completely accurate.
Projection of point (1,1) onto the line (-2,1) amd (2,1) returns a point that
has a distance of 50m from (1,1). Unable to verify if this distance is actually
accurate or not.
Comment thread geojson/geojson_shapes_impl.go Outdated
Comment thread geojson/geojson_shapes_impl.go Outdated
Comment thread geojson/geojson_shapes_impl.go

@CascadingRadium CascadingRadium left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just the one comment otherwise LGTM

@abhinavdangeti

abhinavdangeti commented May 5, 2025

Copy link
Copy Markdown
Member

I like @Likith101 's recommendation of limiting or refraining from API changes to s2/ to keep merge conflicts to a minimum in the future when we update our fork - that said have @CascadingRadium and @Likith101 come to a common ground on the conversation already?

@Likith101 Likith101 merged commit 49aee82 into master May 6, 2025
@abhinavdangeti abhinavdangeti deleted the points branch May 6, 2025 13:59
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