Spatial join area #1713
Spatial join area #1713
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1713 +/- ##
==========================================
+ Coverage 90.03% 90.08% +0.04%
==========================================
Files 396 396
Lines 37968 38148 +180
Branches 4261 4279 +18
==========================================
+ Hits 34186 34364 +178
+ Misses 2492 2491 -1
- Partials 1290 1293 +3 ☔ View full report in Codecov by Sentry. |
src/engine/SpatialJoinAlgorithms.cpp
Outdated
| } catch (...) { | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
does boost geometry have a mode or overload of readWkt that doesn't use exceptions? then use that.
There was a problem hiding this comment.
No. At least I'm not aware of such a function or overload and could not find one.
src/engine/SpatialJoinAlgorithms.cpp
Outdated
| // ____________________________________________________________________________ | ||
| double SpatialJoinAlgorithms::convertDegreesToMeters( | ||
| AnyGeometry geometry1, AnyGeometry geometry2) const { | ||
| return boost::apply_visitor(DistanceVisitor(), geometry1, geometry2) * 78.630; |
There was a problem hiding this comment.
What exactly is the multiplication doing here, the magic constant should be explained.
Is the function name actually computeDistanceInMeters (you first compute it, and then do some magic multiplication to convert)
There was a problem hiding this comment.
a very verbose explanation of this is in the header file. But you're right, the function should be called different, as it first gets the distance in degrees and then converts it. I called it computeDist, since that's what the other compute distance functions are called. Then it's consistent and depending on the parameters available, one can call one or the other computeDist function
There was a problem hiding this comment.
Thank you for the clarification, I am much less confused now:)
src/engine/SpatialJoinAlgorithms.cpp
Outdated
| AnyGeometry geom1 = geo1.geometry.value(); | ||
| AnyGeometry geom2 = geo2.geometry.value(); | ||
| if (geo1.geoPoint.has_value()) { | ||
| geom1 = convertGeoPointToPoint(geo1.geoPoint.value()); | ||
| } else if (geo2.geoPoint.has_value()) { |
There was a problem hiding this comment.
What is stored e.g. in geo1.geometry.value() if geo1 points to a POINT already? This last part looks a bit confusing.
There was a problem hiding this comment.
one line above, there is a comment, which states "one point and one area". So this is the case, where we have one GeoPoint and one area. Then geom1 and geom2 get initialized to be the geometries (which is wrong in one case. When the geometry is a GeoPoint, then geomX.geometry is std::nullopt). The if below checks, which of those two is not a geometry but a geopoint. Then it converts geopoint to boost point (as boost requires both types to be boost geometries). You're right, that they should not be initialized with .value, since that is std::nullopt for at least one of them. I still have to write a test, which checks that, but it should work now
src/engine/SpatialJoinAlgorithms.cpp
Outdated
| boost::geometry::read_wkt(getAreaString(idtable, row, col), geometry); | ||
| } else { | ||
| geometry = Point(point.value().getLng(), point.value().getLat()); | ||
|
|
There was a problem hiding this comment.
Easier control flow:
if (point.has_value) { return point; } else { return getAnyGeometry(...)} Everything else then is not nested in the if`.
There was a problem hiding this comment.
'(with my suggestion for the AnyGeometry)
There was a problem hiding this comment.
i don't understand what you mean
There was a problem hiding this comment.
if (point.has_value()) {
return std::optional{AnyGeometry{convertPoint...()}};
}
return std::optional{getAnyGeometry(...);
You can drop the std::optional if you explicitly set the return type of the lambda to std::optional<AnyGeometry>
There was a problem hiding this comment.
Of course only relevant, if we still have this code at all.
src/engine/SpatialJoinAlgorithms.cpp
Outdated
| // ____________________________________________________________________________ | ||
| double SpatialJoinAlgorithms::convertDegreesToMeters( | ||
| AnyGeometry geometry1, AnyGeometry geometry2) const { | ||
| return boost::apply_visitor(DistanceVisitor(), geometry1, geometry2) * 78.630; |
There was a problem hiding this comment.
Thank you for the clarification, I am much less confused now:)
src/engine/SpatialJoinAlgorithms.cpp
Outdated
| boost::geometry::read_wkt(getAreaString(idtable, row, col), geometry); | ||
| } else { | ||
| geometry = Point(point.value().getLng(), point.value().getLat()); | ||
|
|
There was a problem hiding this comment.
if (point.has_value()) {
return std::optional{AnyGeometry{convertPoint...()}};
}
return std::optional{getAnyGeometry(...);
You can drop the std::optional if you explicitly set the return type of the lambda to std::optional<AnyGeometry>
src/engine/SpatialJoinAlgorithms.cpp
Outdated
| const auto& areaBox = bbox.value(); | ||
| Point p = calculateMidpointOfBox(areaBox); | ||
| return GeoPoint(p.get<1>(), p.get<0>()); |
There was a problem hiding this comment.
I think below the code gets much more readable if you do
if (!useMidpoint) { return computeDist(index1, index2);}; //.
Then it becomes much clearer that the whole complexity is just for the midpoint case (is this actually useful still?)
If you want to keep the midpoint algorithm then you can move the code into a separate function computeDistUsingMidpoints and then the actual computeDist becomes very short and readable.
There was a problem hiding this comment.
i don't get what you mean. The if else code blocks in the current implementation are equally large and complex for both cases.
Conformance check passed ✅No test result changes. |
|
This merge request adds the possibility to use arbitrary WKT Geometries (i.e. areas) in SpatialJoin queries. Right now, areas can get approximated as the center point or the true distance between them is estimated. In addition to that a future PR will add pre computed bounding boxes to the index generation to enable even faster query times (like it already exists for geopoints)