IntegerQuantity's multiply(Number) has surprising and bug-prone behavior about indriya HOT 15 CLOSED

If you try to pass a number that's e.g. a Double to an Integer operation, that always causes precision loss, that's nothing specific to this library, it happens in every other place. IntegerQuantity is specific to int/Integer values. If you pass a double to an int you lose the decimal part.

If you pass the same data types e.g. both double (or in the SE implementation even BigDecimal, the RI is restricted so in certain situations it may also lose precision compared to the SE equivalent, but it is meant to run on Java ME with the same behavior as on Java SE, therefore BigDecimal is not supported here) you'll retain the greatest possible precision.

from indriya.

Hrm, that's not really my point. Obviously there's going to be some precision loss. The question is whether or not it's consistent with what a normal person would expect, given what they know about how Java works.

This isn't consistent with Java's (or any C-style language's) semantics overall. To the extent that an IntegerQuantity should behave like a "tagged" integer, then int * double should be a double. That seems like a fairly straightforward semantics to have?

My point is that this is going to be a source of silent bugs, because you can't actually trust a Quantity to support meaningful scalar operations, and so if you're doing any actual calculations with Quantities for basically anything that involves multiplication by scalars, you need to unwrap it, turn it into a double, and then rewrap it.

If the result has to be an int, then surely it should be (int)(int * double), and not truncating before hand?

from indriya.

(This is especially pernicious mainly because there's absolutely no static way to tell the difference between the two sorts of quantities.)

from indriya.

Checking e.g. with instanceof should tell if a Quantity is of a particular type.

from indriya.

IntegerQuantity is default access, so you can't do that.

from indriya.

but really, isn't the point of a units library to prevent people from making dumb numerical errors?

from indriya.

It's only visible in the same package, but those are implementation details. You create instances via QuantityFactory or NumberFactory.of(). Except for obvious cases like division (where 1/2 etc. usually produces a decimal result) other operations are not meant to produce different types. If you deal with int, you should be aware of the consequences or do so for a good reason. Otherwise you may (or should) always use double or float at least on Java ME compliant platforms.

from indriya.

from indriya.

That is why numeric type is preserved whenever possible. If we forced using a doubleValue() instead of the consistent intValue() (or longValue() for LongQuantity) or even did so based on the type of number, it could change on the fly. NumberQuantity.of() gives a rather fine-grained control. If you pass it an int or a double, you know what the result will be.

from indriya.

The basic Number types like Long, Integer, Double, etc. are all final in the JDK, so even trying to guess makes no sense. There are hundreds of classes all extending Number in the JDK itself or other frameworks like Google Guava or Apache (e.g. Commons Math, etc.)
Allowing to pass a Number to most of the key operations makes the API flexible and open to most of these libraries and frameworks. The method needs to make some assumption about the value, so right now the rule is, that depending on whether it's Int, Long, Float or Double, the respective methods of Number are used. Although in theory all devices should support every Number type on Java ME, the embedded space may even prefer using int for certain calculations knowing that no decimal will be required. We would not like to take that away by forcing every single operation and Quantity type to use double only.

from indriya.

There are tests like IntegerQuantityTest for this particular quantity type. Would you be willing to provide test cases for the multiply() method or the class as a whole based on your problem statement?
Right now all 287 unit tests of the RI pass. We have to keep it that way and carefully consider e.g. changing the return type even for an internal representation like this class.

from indriya.

I'd be happy to.

To be clear, I am absolutely not suggesting that you require double only. I was originally suggesting that the semantics of IntegerQuantity::multiply be changed from:

  public Quantity<Q> multiply(Number that) {
    return NumberQuantity.of(value * that.intValue(), getUnit());
  }

to something more like:

public Quantity<Q> multiply(Number that) {
    return NumberQuantity.of(GenericMath.multiply(value, that), getUnit());
  }

where GenericMath does a switch on the builtins (maybe with a registry or something for extensibility). However I see that that's probably a bit of a bear.

And I suppose I understand the desire to have the result's type be the same as the LHS's type (whenever possible). But it does seem like the semantics should still be as close as possible to "the closest number representable by the LHS's type", which I think argues for:

public Quantity<Q> multiply(Number that) {
    return NumberQuantity.of((int)(value * that.doubleValue()), getUnit());
  }

from indriya.

We could try out something like

public Quantity<Q> multiply(Number that) {
    return NumberQuantity.of((int)(value * that.doubleValue()), getUnit());
  }

to see how it affects all existing unit tests.

The question is, if we went down that path, whether or not

  public Quantity<?> divide(Quantity<?> that) {
    return NumberQuantity.of((double) value / that.getValue().doubleValue(), getUnit().divide(that.getUnit()));
  }

was OK to cast as double, or it should also for consistency use int? In a number of cases it may lead to 0 values, but it would be consistent with the multiply() operation.

from indriya.

This should be fixed in 385, but transferring it just in case.

from indriya.

Based on #220 I'm closing this.

from indriya.