Should division of integer measurements return an integer or a rational?

[sander-deryckere]

I bumped into an issue when calculating the BMI.

The BMI is weight in kg divided by height in m. But in our storage, we store weight in grams and height in cm. Both are stored as integers.

So I thought to calculate the BMI as following:

> weight = Unitwise(73_500, "g")
#<Unitwise::Measurement value=73500 unit=g>
> height = Unitwise(175, "cm")
#<Unitwise::Measurement value=175 unit=cm>
> bmi = (weight / (height ** 2)).convert_to("kg/m2")
#<Unitwise::Measurement value=20 unit=kg/m2>

If you do the calculation, you'll notice that the BMI should be 24 instead of 20. What happens is that it divides 73,500 by 175^2 (=30,625), as integers. This rounds to 2 g/cm2. When converted to kg/m2, it becomes the 20 you see.

Fixing this issue was somewhat annoying. The values come from our storage layer as Unitwise objects, so just putting random .to_f or .to_r calls before wrapping it in Unitwise isn't desirable.

In the end, the easiest workaround was to multiply it with a float: (1.0 * weight / (height ** 2)).convert_to("kg/m2")

Unitwise already does some automatic conversions. Like this:

> Unitwise(2, "g").convert_to("kg")
#<Unitwise::Measurement value=1/500 unit=kg>

So this issue took me by surprise. I had expected to receive a float or a rational.

Integer division is sometimes handy when programming. But mainly when counting and calculating indices of some sort. I don't think it's ever what people expect when dividing quantities.

[saraid]

The following change appears to address this issue:

diff --git a/lib/unitwise/measurement.rb b/lib/unitwise/measurement.rb
index f8541e5..509f0e5 100644
--- a/lib/unitwise/measurement.rb
+++ b/lib/unitwise/measurement.rb
@@ -185,7 +185,7 @@ module Unitwise
           new(value.send(operator, converted.value),
               unit.send(operator, converted.unit))
         else
-          new(value.send(operator, other.value),
+          new(to_r.send(operator, other.value),
               unit.send(operator, other.unit))
         end
       end

I imagine a few other cases need such a treatment (such as the other 2 conditional branches in this method), but I only stumbled upon this library tonight and don't really feel confident in trying to make a substantial change when I'm not really familiar with what normal behavior looks like (the tests don't appear to pass, for me, but I'm not on ruby2.4). Hopefully this is adequate for you to build off, though.

[sander-deryckere]

This would mean all operations, including +, - and *, are cast to a rational. I don't know enough of this library to know if this is acceptable or wanted behaviour.

[saraid]

Agreed.

You could also do

def value_for_operation(operator)
  case operator
  when :/ then to_r
  else value
  end
end

and then change it to

new(value_for_operation(operator).send(operator, other.value),
    unit.send(operator, other.unit))

So there are options. Like you, I'm not familiar enough to pick a strategy.