Use Rationals for intermediate conversion values

trishume commented 9 years ago

Fixes #4 to some extent.

Problem

Measured builds a conversion table in Measured::ConversionTable but some values in the table would be wrong, for example the conversion value from feet to yards would not be 3. The reason for this is Measured's tree traversal would go through the base unit ft->m->yd using BigDecimal for intermediate values but the m->yd can not be done with perfect precision with decimal numbers.

Solution

Use Rationals for intermediate values when building the conversion table. This means all intermediate multiplications will be done with perfect precision. Currently this fixes all the previously skipped tests.

Todo

[x] Don't use new Ruby 2.2 round(n) feature so that CI can pass
[x] Always give back Rational from inverse_conversion_amount because BigDecimal can't do this
Commentary

Really this issue stems from the fact that BigDecimal is used at all. In a perfect world the library would only ever use Rational because it is always perfect and never imprecise :sparkles:. By using BigDecimal we accept that some of our conversions will always be lossy, not to mention we will never be able to represent "1/3 of a meter" accurately.

I'm not sure why the library was designed to use BigDecimal at all, but IMO it might be worthwhile to make Measured only support Rational to avoid these issues. If the issue is storage space in the DB then we could always make Measured::Rails do the conversion to/from BigDecimal at the persistence level.

@kmcphillips @cyprusad @garethson

trishume commented 9 years ago

Ok so I fiddled with some things and pushed the BigDecimal out to the very end of the conversion process for maximum accuracy. This means that all conversions now give a result that is accurate to ARBITRARY_CONVERSION_PRECISION which I arbitrarily set to 20 (hence the name).

kmcphillips commented 9 years ago

This is stellar work. Great solution.

kmcphillips commented 9 years ago

Can you bump the version too?

kmcphillips commented 9 years ago

And I do agree with you. We can look at making Rational the core type for Measured and do the conversion to BigDecmal in the rails adapter.

trishume commented 9 years ago

Bumped version to v0.0.12. Ready to :ship:?

cyprusad commented 9 years ago

:sparkling_heart:

trishume commented 9 years ago

I realized that I removed a property from these tests that should be there. I just pushed a commit that asserts exact (no threshold) equality wherever perfect conversion is possible.

kmcphillips commented 9 years ago

asserts exact (no threshold) equality wherever perfect conversion is possible

I was thinking about this exact situation last night! Thanks. When we know we should do exact conversion we should assert as such. So great.

kmcphillips commented 9 years ago

Yeah, I'm good with this. :+1:

Shopify / measured