Skip to content

Conversation

@paleolimbot
Copy link
Member

Updates the geo crate to 0.32.0

Closes #455

Closes #491

Replaces #549

Comment on lines +45 to +48
///
/// 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).
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Comment on lines +1574 to +1595
// #[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
// );
// }
// }
Copy link
Member Author

Choose a reason for hiding this comment

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

@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)

Copy link
Member

Choose a reason for hiding this comment

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

Preliminary investigations indicate that our calculations are correct, and geo 0.32.0 is incorrect.

I'll look into it further and see if I can submit a patch to georust/geo.

Copy link
Member

Choose a reason for hiding this comment

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

I have submitted georust/geo#1499 to fix this.

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.

rust/sedona-geo: Upgrade to geo 0.32

2 participants