geo

geo copied to clipboard

Mostly a feature request that I'm interested in hearing your take on. It's quite common for me to have failed unit tests that check assert_eq! on structs with geometries, simply because of floating point rounding errors.

I don't think we should change PartialEq to be approximate, because I think this would surprise lots of people.

Instead, have you seen our usage of approx? There's probably some unit tests within our own repos that aren't yet using approx, but they should be if you feel like contributing.

We already have https://docs.rs/geo/latest/geo/geometry/enum.Geometry.html#impl-RelativeEq%3CGeometry%3CT%3E%3E-for-Geometry%3CT%3E, right?

Maybe I'm being a bit misunderstood. Take this struct for example:

pub struct AddressSearch {
    id: Id,
    address_string: String,
    postal_info: Option<PostalInfoReference>,
    geometry: Point,
}

How would you go about unit testing equality of an expected value to an REST API call? Do I really want to check the equality of each and every field? What happens then when AddressSearch is included in another struct? It just doesn't scale.

A temporary solution that I'm currently using is manually implementing PartialEq where the floating point fields use approx:

impl PartialEq for AddressSearch {
    fn eq(&self, other: &Self) -> bool {
        self.id == other.id
            && self.address_string == other.address_string
            && self.postal_info == other.postal_info
            && approx::relative_eq!(self.geometry, other.geometry, epsilon = 1e-6)
    }
}

Being able to override PartialEq with an opt-in feature flag makes this decision explicit, so it should not result in surprises by the user.

Overriding PartialEq in this crate was a "solution" that I thought was adequate, but probably not ideal. It might be worth attempting to solve this on a more general level with a separate derive macro. Maybe I should turn my efforts to approx?

Yeah, floating point makes comparisons annoying. It's not so much conceptually difficult, but it makes writing test code very verbose which is a pain.

But people assume a contract with PartialEq, and I think your suggested usage breaks that contract. For example, PartialOrd depends on the semantics of PartialEq. So I think implementing PartialEq like you're suggesting has ramifications on sorting, which could affect our other algorithms. If you want to do that in your own code, that's probably fine, but I don't think it's something we should expose, even as an optional feature.

If you want to be able to approximately compare your structs, I'd recommend implementing the approx::RelativeEq for your structs as well, like we have for the geometries in geo-types.