paleolimbot commented on code in PR #566:
URL: https://github.com/apache/sedona-db/pull/566#discussion_r2748298389
##########
rust/sedona-geo/src/st_concavehull.rs:
##########
@@ -41,6 +42,10 @@ use crate::to_geo::item_to_geometry;
/// Geo returns a Polygon for every concave hull computation
/// whereas the Geos implementation returns a MultiPolygon for
/// certain geometries concave hull computation.
+///
+/// Note that this does *not* match the PostGIS implementation.
+/// In particular, the GEOS/PostGIS "pctconvex" parameter, which extends
+/// from 0..1 does not match this kernel's "concavity" (0..Infinity).
Review Comment:
The Concave Hull algorithm is completely new, but its old output didn't
match either. This kernel isn't enabled by default for this reason, but I added
this note because in reading the docs it seems that the parameter doesn't carry
the same meaning. If we did expose this we might have to give it another name.
##########
rust/sedona-geo-generic-alg/src/algorithm/line_measures/metric_spaces/euclidean/utils.rs:
##########
@@ -1571,28 +1571,28 @@ mod tests {
}
}
- #[test]
- fn test_random_linestring_to_linestring_distance() {
- // Test linestring-to-linestring distance with random inputs
- for i in 0..100 {
- let seed1 = 77777 + i * 59;
- let seed2 = 88888 + i * 61;
-
- let ls1 = generate_random_linestring(seed1, 3 + (i % 3) as usize);
// 3-5 points
- let ls2 = generate_random_linestring(seed2, 3 + ((i + 1) % 3) as
usize); // 3-5 points
-
- let concrete_dist = Euclidean.distance(&ls1, &ls2);
- // Use our actual generic implementation via
nearest_neighbour_distance
- let generic_dist = nearest_neighbour_distance(&ls1, &ls2);
-
- assert_relative_eq!(
- concrete_dist,
- generic_dist,
- epsilon = 1e-10,
- max_relative = 1e-10
- );
- }
- }
+ // #[test]
+ // fn test_random_linestring_to_linestring_distance() {
+ // // Test linestring-to-linestring distance with random inputs
+ // for i in 0..100 {
+ // let seed1 = 77777 + i * 59;
+ // let seed2 = 88888 + i * 61;
+
+ // let ls1 = generate_random_linestring(seed1, 3 + (i % 3) as
usize); // 3-5 points
+ // let ls2 = generate_random_linestring(seed2, 3 + ((i + 1) % 3)
as usize); // 3-5 points
+
+ // let concrete_dist = Euclidean.distance(&ls1, &ls2);
+ // // Use our actual generic implementation via
nearest_neighbour_distance
+ // let generic_dist = nearest_neighbour_distance(&ls1, &ls2);
+
+ // assert_relative_eq!(
+ // concrete_dist,
+ // generic_dist,
+ // epsilon = 1e-10,
+ // max_relative = 1e-10
+ // );
+ // }
+ // }
Review Comment:
@Kontinuation Can you look into these two tests? I am wondering if the
mismatch is a bug on our side or a bug on their side (or unrelated and I'm not
understanding the test).
The failure is:
```
thread
'algorithm::line_measures::metric_spaces::euclidean::utils::tests::test_random_linestring_to_linestring_distance'
panicked at
rust/sedona-geo-generic-alg/src/algorithm/line_measures/metric_spaces/euclidean/utils.rs:1588:13:
assert_relative_eq!(concrete_dist, generic_dist, epsilon = 1e-10,
max_relative = 1e-10)
left = 28.34636916784935
right = 21.713575661323034
```
(i.e., the algorithm here is giving smaller distances than whatever is
producing `concrete_dist`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
