Spatial join bounding box #1546
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1546 +/- ##
==========================================
+ Coverage 89.24% 89.34% +0.09%
==========================================
Files 374 375 +1
Lines 35683 35925 +242
Branches 4027 4063 +36
==========================================
+ Hits 31845 32096 +251
+ Misses 2538 2533 -5
+ Partials 1300 1296 -4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Jonathan24680
force-pushed
the
SpatialJoinBoundingBox
branch
from
a9eb289 to
e9ed3e4
Compare
src/engine/SpatialJoinAlgorithms.h
Outdated
| std::vector<box> OnlyForTestingWrapperComputeBoundingBox( | ||
| const point& startPoint) const { | ||
| return computeBoundingBox(startPoint); | ||
| } | ||
|
|
||
| bool OnlyForTestingWrapperContainedInBoundingBoxes( | ||
| const std::vector<box>& bbox, point point1) const { | ||
| return containedInBoundingBoxes(bbox, point1); | ||
| } |
There was a problem hiding this comment.
As they are const, and thus safe to use, you don't need those funnily named wrappers, but just can make the called functions public. Maybe some of them can even be static.
There was a problem hiding this comment.
i want the names to convey that these methods are not supposed to get called by the user of the class and that he does not need to care what those methods do. Otherwise i could just make the two wrapped functions public and completely avoid the wrapping altogether. Don't you think it is confusing for users of the class?
|
|
||
| // testing only possible, if lat bounds are correct and the lon bounds | ||
| // don't cover everything (as then left or right of the box is again | ||
| // inside the box because of the spherical geometry) |
There was a problem hiding this comment.
And what do we have to check in that case? Or can't we do anything?
There was a problem hiding this comment.
If we have a bounding box, which goes from -180 to 180 longitude, then left of the bounding box is just in the bounding box again. (i.e. -180.00001 is the same as +179.99999). As all longitudes are covered, a left or right bound does not exist (on the sphere this makes intuitive sense). A test in that case is not necessary, because this test is about testing the edges and if everything is covered an edge doesn't exist there is no need for testing in that case
I added the above explanation to the code
| // broad grid test | ||
| for (int lon = -180; lon < 180; lon += 20) { | ||
| for (int lat = -90; lat < 90; lat += 20) { | ||
| checkOutside(point(lon, lat), startPoint, bbox, &spatialJoinAlgs); | ||
| } | ||
| } |
There was a problem hiding this comment.
Probably you should choose some distribution that has more values closer to the startPoint, because that's where it is interesting.
There was a problem hiding this comment.
i think that's not necessary as the testBounds function is doing exactly that: heavily testing the values barely in or out of the bounding box. This function checks everything but that so the combination covers everything.
| } | ||
| } | ||
|
|
||
| } // namespace boundingBox |
There was a problem hiding this comment.
I didn't yet look at all those tests in detail,
but something you could also do:
Get a bounding box from your implementation, and get a bounding box via S2, and then
check that they are consistent etc.
There was a problem hiding this comment.
yes, that's a possibility. Although i think the current tests are already sufficient. If you want to i can implement it though.
Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
| rightJoinCol, numColumns, maxDist, maxResults] = params_; | ||
| IdTable result{numColumns, qec_->getAllocator()}; | ||
|
|
||
| // create r-tree for smaller result table |
Conformance check passed ✅No test result changes. |
|
We now have two efficient implementations of spatial joins: One using Google's S2 library, and now a new one using
boost::geometry::rtree. Currently both of these implementations are limited to point geometries, the new, rtree-based implementation is easier to extend to arbitrary geometries, which will be implemented soon.As boost::geometry purely works on Cartesian coordinates, which are especially unsuited for the spherical form of the earth, this PR manually implements the required expansion of bounding boxes to also show the correct behavior for bounding boxes that cross one of the poles or the 180th degree of longitude.