iliekturtles / uom

Units of measurement -- type-safe zero-cost dimensional analysis
Apache License 2.0
1.03k stars 98 forks source link

Unexpected inequality with large conversion factor #441

Open inferiorhumanorgans opened 1 year ago

inferiorhumanorgans commented 1 year ago

With this code:

unit! {
    system: uom::si;
    quantity: uom::si::length;

    @flight_level: 30.48; "FL", "flight level", "flight level";
}

This test fails:

#[test]
fn fight_level() {
    use uom::si::f32::Length;
    use uom::si::length::foot;
    use super::flight_level;

    let fl210 : Length = Length::new::<flight_level>(210.0);
    assert_eq!(fl210, Length::new::<foot>(21000.0));

}
assertion `left == right` failed
  left: 6400.8 m^1
 right: 6400.8003 m^1

This also brings to mind that being having a PartialEq impl would make it possible easier to use e.g. approx to compare values.

iliekturtles commented 1 year ago

The slight difference in the values is a side effect of normalizing the internal value in meters and floating point imprecision. While PartialEq is implemented, I don't believe it will help in this case.

Floating point accuracy problems are always a hassle! You can see how uom's test code uses approx here: https://github.com/iliekturtles/uom/blob/629f385477212c20048aee0faff2079f47a1194b/src/tests/mod.rs#L110-L140

inferiorhumanorgans commented 1 year ago

Oh, whoops. I was the assert_relative_eq macro. The error was about not implementing the crate's RelativeEq trait.

iliekturtles commented 1 year ago

Are you proposing implementing approx::RelativeEq inside uom? If we're going to expose approx in uom's public interface I'd want to review other options. In a very brief scan I also found float-cmp and assert_approx_eq. All three seem pretty popular. I'm hesitant to pick a winner, but the orphan rules sort of require doing that.

adamreichold commented 1 year ago

I'm hesitant to pick a winner, but the orphan rules sort of require doing that.

Could you expand on why that is the case? Couldn't uom theoretically implement the traits from all three crates (and in different versions if required)? Also even if uom provides an implementation of only one trait, can't downstream crates still implement the other using newtype wrappers just like they have to do now for any of them? (Which would even allow overriding a provided implementation with the same effort as implementing it in the first place now.)

iliekturtles commented 1 year ago

I hadn't thought about the opposite and adding multiple implementations. Similar question though, when should the line be drawn on which external crates should be implemented?

adamreichold commented 1 year ago

Similar question though, when should the line be drawn on which external crates should be implemented?

Where you as the maintainer want it to be drawn from an effort versus convenience perspective. Since the basic set of implementable traits is unaffected, the stakes for answering this are not that high, i.e. you can choose one that you like to provide a convenient path for some users while not blocking anything for others.

Of course, just saying no is also a maintainer's prerogative and providing none also does not block anyone from using newtype wrappers. My point basically is that the orphan rules only enforce one thing here: This crate is the only place where we can provide these trait implementations without the additional effort of newtype wrappers.