Surprising behaviour when comparing non-metric units

[nsunderland1]

I have the following code, which defines a foot/pound/second system of units:

#[macro_use]
extern crate uom;

ISQ!(
    uom::si,
    f64,
    (foot, pound, second, ampere, degree_rankine, mole, candela)
);

#[cfg(test)]
mod tests {
    #[test]
    fn equal() {
        let value = super::Length::new::<uom::si::length::foot>(28.0);

        assert_eq!(value, value);
    }
}

My expectation is that the test will pass, as value should be equal to itself.

Instead, I get the following error:

thread 'tests::equal' panicked at src/lib.rs:16:9:
assertion `left == right` failed
  left: 28.0 ft^1
 right: 28.0 ft^1

This is surprising because:

• Outside of NaN, I expect float values to equal themselves.
• The panic message is confusing, because there's no obvious difference between the values.
• If you instead compare value.value, or value.get::<foot>(), etc. the test passes.

I looked into Quantity's PartialEq implementation, and saw that it calls change_base on the right operand to handle the fact that the operand types can have different U parameters. change_base looks like this:

fn change_base<D, Ul, Ur, V>(v: &V) -> V
where
    D: Dimension + ?Sized,
    Ul: Units<V> + ?Sized,
    Ur: Units<V> + ?Sized,
    V: crate::Conversion<V> + crate::lib::ops::Mul<V, Output = V>,
{
    use crate::typenum::Integer;
    use crate::{Conversion, ConversionFactor};
    (v.conversion() * Ur::length::coefficient().powi(D::L::to_i32())
        / Ul::length::coefficient().powi(D::L::to_i32())
        * Ur::mass::coefficient().powi(D::M::to_i32())
        / Ul::mass::coefficient().powi(D::M::to_i32())
        * Ur::time::coefficient().powi(D::T::to_i32())
        / Ul::time::coefficient().powi(D::T::to_i32())
        * Ur::electric_current::coefficient().powi(D::I::to_i32())
        / Ul::electric_current::coefficient().powi(D::I::to_i32())
        * Ur::thermodynamic_temperature::coefficient().powi(D::Th::to_i32())
        / Ul::thermodynamic_temperature::coefficient().powi(D::Th::to_i32())
        * Ur::amount_of_substance::coefficient().powi(D::N::to_i32())
        / Ul::amount_of_substance::coefficient().powi(D::N::to_i32())
        * Ur::luminous_intensity::coefficient().powi(D::J::to_i32())
        / Ul::luminous_intensity::coefficient().powi(D::J::to_i32()))
    .value()
}

Essentially since I'm not using metric units, we end up having something that looks like this:

// 0.3048 being the ft -> m conversion factor
self.value * 0.3048 / 0.3048 * ...

These operations don't cancel out due to floating point error, so we end up with a different value, hence the assertion failure.

I'm not sure what uom could do to handle this better, but it is unfortunate that these conversions have to happen at all when comparing values in the same unit system. There's probably a performance overhead to that as well?

uom version: 0.36.0

I have a minimal repro of the issue here: https://github.com/nsunderland1/uom-partialeq

[nsunderland1]

Worth mentioning that disabling the autoconvert feature fixes this since the code for handling mixed base units is stripped out, though the downside to that solution is that it can easily be broken by feature unification.

[iliekturtles]

I put a lot of time into to_base and from_base [1] to resolve similar floating point precision issues, but haven't done the same kind of work for change_base. To resolve this issue a test case that triggers the issue should be added and then changes should be made to change_base to minimize floating point precision issues as much as possible while still maintaining zero-cost generated code (above what manually written conversions would look like).

[1]

uom/src/system.rs

Lines 297 to 357 in d385d79

	/// Convert a value from base units to the given unit.
	///
	/// ## Generic Parameters
	/// * `D`: Dimension.
	/// * `U`: Base units.
	/// * `V`: Value underlying storage type.
	/// * `N`: Unit.
	#[inline(always)]
	fn from_base<D, U, V, N>(v: &V) -> V
	where
	D: Dimension + ?Sized,
	U: Units<V> + ?Sized,
	V: $crate::Conversion<V>,
	N: $crate::Conversion<V, T = V::T>,
	{
	use $crate::typenum::Integer;
	use $crate::{Conversion, ConversionFactor};
	let v = v.conversion();
	let n_coef = N::coefficient();
	let f = V::coefficient() $(* U::$name::coefficient().powi(D::$symbol::to_i32()))+;
	let n_cons = N::constant($crate::ConstantOp::Sub);
	if n_coef < f {
	(v * (f / n_coef) - n_cons).value()
	}
	else {
	(v / (n_coef / f) - n_cons).value()
	}
	}
	/// Convert a value from the given unit to base units.
	///
	/// ## Generic Parameters
	/// * `D`: Dimension.
	/// * `U`: Base units.
	/// * `V`: Value underlying storage type.
	/// * `N`: Unit.
	#[inline(always)]
	fn to_base<D, U, V, N>(v: &V) -> V
	where
	D: Dimension + ?Sized,
	U: Units<V> + ?Sized,
	V: $crate::Conversion<V>,
	N: $crate::Conversion<V, T = V::T>,
	{
	use $crate::typenum::Integer;
	use $crate::{Conversion, ConversionFactor};
	let v = v.conversion();
	let n_coef = N::coefficient();
	let f = V::coefficient() $(* U::$name::coefficient().powi(D::$symbol::to_i32()))+;
	let n_cons = N::constant($crate::ConstantOp::Add);
	if n_coef >= f {
	((v + n_cons) * (n_coef / f)).value()
	}
	else {
	(((v + n_cons) * n_coef) / f).value()
	}
	}