uom icon indicating copy to clipboard operation
uom copied to clipboard

Unexpected inequality with large conversion factor

Open inferiorhumanorgans opened this issue 10 months ago • 6 comments

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.

inferiorhumanorgans avatar Oct 21 '23 03:10 inferiorhumanorgans

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

iliekturtles avatar Oct 23 '23 20:10 iliekturtles

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

inferiorhumanorgans avatar Oct 24 '23 21:10 inferiorhumanorgans

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.

iliekturtles avatar Oct 25 '23 13:10 iliekturtles

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

adamreichold avatar Oct 25 '23 16:10 adamreichold

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?

iliekturtles avatar Nov 03 '23 14:11 iliekturtles

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.

adamreichold avatar Nov 03 '23 15:11 adamreichold