georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.49k stars 194 forks source link

Override `PartialEq` derives when a `approx-partial-eq` feature flag is enabled. #1085

Open gibbz00 opened 9 months ago

gibbz00 commented 9 months ago

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.

michaelkirk commented 9 months ago

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.

lnicola commented 9 months ago

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

gibbz00 commented 9 months ago

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?

michaelkirk commented 9 months ago

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.