unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
118 stars 40 forks source link

Don't accept non-finite amounts with Quantity constructors #285

Closed DrDaleks closed 4 years ago

DrDaleks commented 4 years ago

Currently using version 2.0.2.

The following line

Quantities.getQuantity(Double.POSITIVE_INFINITY, Units.METRE).to(MetricPrefix.KILO(Units.METRE));

Leads to the following stack trace

java.lang.NumberFormatException
    at java.math.BigDecimal.<init>(BigDecimal.java:497)
    at java.math.BigDecimal.<init>(BigDecimal.java:383)
    at java.math.BigDecimal.<init>(BigDecimal.java:809)
    at java.math.BigDecimal.valueOf(BigDecimal.java:1277)
    at tech.units.indriya.function.DefaultNumberSystem.multiplyWideAndNarrow(DefaultNumberSystem.java:761)
    at tech.units.indriya.function.DefaultNumberSystem.multiply(DefaultNumberSystem.java:166)
    at tech.units.indriya.internal.function.calc.Calculator.multiply(Calculator.java:114)
    at tech.units.indriya.function.PowerOfIntConverter.convertWhenNotIdentity(PowerOfIntConverter.java:151)
    at tech.units.indriya.function.AbstractConverter.convert(AbstractConverter.java:236)
    at tech.units.indriya.AbstractQuantity.to(AbstractQuantity.java:199)
andi-huber commented 4 years ago

Could be considered correct behavior. Or what would you expect instead?

DrDaleks commented 4 years ago

In mathematical terms, isn't infinity always infinity (and therefore returned as such) ? The only caveat would be to account for sign (in case a conversion happens to inverse it)

In any case, getting a NumberFormatException isn't logical here. IllegalArgument perhaps (much further upstream) if you do consider this bad behaviour. But going through the inner guts indicates a missing corner case.

keilw commented 4 years ago

It really is a corner case, @andi-huber do you see a realistic chance to deal with that in classes like Calculator, or is it mostly up to BigDecimal anyway and we just have to accept it?

andi-huber commented 4 years ago

Agreed, if there are no side-effects, we could consider having Quantities.getQuantity(Double.POSITIVE_INFINITY, ...) throw an IllegalArgumentException.

Just to be clear on infinities: we cannot do any arithmetic on them, hence its sane to not allow numbers that do not represent finite values.

I'll see what I can do, thx for spotting this.

keilw commented 4 years ago

Ok thanks, I think IllegalArgumentException is appropriate as we throw it in other places in similar situations.

DrDaleks commented 4 years ago

No worries. Not sure why returning infinity wouldn't be an option, but the illegal argument is also a valid approach (that's more checks on client side)

andi-huber commented 4 years ago

eg. not defined: inf + (-inf) inf / inf

andi-huber commented 4 years ago

For any call to Quantities.getQuantity(Number amount, Unit unit), given amount is passed over to the configured NumberSystem where it is now narrowed. This allows us to do a check for finite numbers within our DefaultNumberSystem. If a non-finite number is discovered, we throw an IllegalArgumentException.

Users may provide their own NumberSystem to override this behavior.