Bugzilla – Full Text Bug Listing
| Summary: | Conversion problem for metric units with exponents (l to m3) | ||
|---|---|---|---|
| Product: | [Technology] UOMo | Reporter: | Peter Melnikov <peter.melnikov> |
| Component: | UCUM | Assignee: | Grahame Grieve <grahame> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | peter.melnikov, werner.keil |
| Version: | 0.6.0 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
What happens in the Converter is: norm finished: m3 normalise: m3 norm finished: m3 1000l =(0.1)= 100.0m3 =(1)= 100.0m3 In fast the destination value is 1, but source value 0.1, so part of your assumption seems correct. Have to check, where the 0.1 comes from. (In reply to comment #2) > In fast the destination value is 1, but source value 0.1, so part of your > assumption seems correct. Have to check, where the 0.1 comes from. I have almost lost in unit converting classes logic but... should not the line (org.eclipse.uomo.ucum.canonical.Converter.java:201): if (comp.hasPrefix()) { 201: ctxt.multiplyValue( comp.getPrefix().getValue() ); } Be instead: if (comp.hasPrefix()) { ctxt.multiplyValue( comp.getPrefix().getValue().pow(comp.getExponent()) ); } At least it makes following conversions correct: L -> m3, dm3, cm3 ml -> cm3, dm3 But I'm not sure if it could break the things in other cases... Does it makes sense? (In reply to comment #3) > (In reply to comment #2) > > In fast the destination value is 1, but source value 0.1, so part of your > > assumption seems correct. Have to check, where the 0.1 comes from. > > I have almost lost in unit converting classes logic but... should not the line > (org.eclipse.uomo.ucum.canonical.Converter.java:201): > > if (comp.hasPrefix()) { > 201: ctxt.multiplyValue( comp.getPrefix().getValue() ); > } > > Be instead: > > if (comp.hasPrefix()) { > ctxt.multiplyValue( comp.getPrefix().getValue().pow(comp.getExponent()) ); > } > > At least it makes following conversions correct: > > L -> m3, dm3, cm3 > ml -> cm3, dm3 > > But I'm not sure if it could break the things in other cases... > > Does it makes sense? Do you think you could provide a patch for that? I know, the concept of patching may not work with Git, but cloning or forking the Git repository should work. If there isn't a real patch file, attaching the files changed to the bug would be an option. TIA Peter, the idea was good, and for UcumServiceTest (package org.eclipse.uomo.ucum.tests) it solves the broken test. Though resulting in a BigDecimal that's not simply 1, but 1.000, thus only this assertion works: assertEquals(fmt.format(BigDecimal.ONE), fmt.format(mult)); using ICU4J DecimalFormat. Had to be a little more precise, by adding MathContext.DECIMAL128 Do you feel this addresses the problem? (In reply to comment #5) > Peter, > the idea was good, and for UcumServiceTest (package > org.eclipse.uomo.ucum.tests) it solves the broken test. > Though resulting in a BigDecimal that's not simply 1, but 1.000, thus only this > assertion works: > assertEquals(fmt.format(BigDecimal.ONE), fmt.format(mult)); > using ICU4J DecimalFormat. > > Had to be a little more precise, by adding MathContext.DECIMAL128 > > Do you feel this addresses the problem? Hi Werner, Sorry for not answering the first message, was really busy. May be it makes sense not to compare the decimal representations but rather BigDecimal to BigDecimal and pass the result of comparing to the assertTrue or whatever else? I believe that BigDecimal.equals() method handles well the equal decimal numbers with the different precision. Or there should be a standard way to remove non-significant zeros... I'm really happy having that major issue solved, because it was just my hitty-missy suggestion of the line where the problem could be. Could you specify in which version of UCUM the fix will be included and whether it has been already committed to the Git, so the others who may be using the code will get the fix as well. (In reply to comment #6) > (In reply to comment #5) > > Peter, > > the idea was good, and for UcumServiceTest (package > > org.eclipse.uomo.ucum.tests) it solves the broken test. > > Though resulting in a BigDecimal that's not simply 1, but 1.000, thus only this > > assertion works: > > assertEquals(fmt.format(BigDecimal.ONE), fmt.format(mult)); > > using ICU4J DecimalFormat. > > > > Had to be a little more precise, by adding MathContext.DECIMAL128 > > > > Do you feel this addresses the problem? > > Hi Werner, > > Sorry for not answering the first message, was really busy. > > May be it makes sense not to compare the decimal representations but rather > BigDecimal to BigDecimal and pass the result of comparing to the assertTrue or > whatever else? I believe that BigDecimal.equals() method handles well the equal > decimal numbers with the different precision. Or there should be a standard way > to remove non-significant zeros... > > I'm really happy having that major issue solved, because it was just my > hitty-missy suggestion of the line where the problem could be. > Could you specify in which version of UCUM the fix will be included and whether > it has been already committed to the Git, so the others who may be using the > code will get the fix as well. Peter, Thanks a lot for the reply. No problem, especially Grahame has been very busy, too otherwise he may have looked into that (I reproduced the problem with the original OHF codebase, too, so it isn't new since UCUM was moved to UOMo) I have not followed the traces all the way back, but BigDecimal's equals() method seems to be a very strange beast. Just like BigInteger and Integer in Java are not the same either! We have another "equals() related" bug, where two or more classes behave like those in the JDK, thus KILO(GRAM), KILOGRAM and "1000 GRAM" may not be considered the same. The problem isn't new to the JDK, as new BigInteger(1) and new Integer(1) are also not equal!?;-) However, to meet requirements of real business cases, not just pass unit tests this may have to change here, too. I'll look into the issue in more detail if I can. The underlying problems seem partly in the JDK and the way, BigDecimal, BigInteger and other Number subclasses behave, especially with regards to the equals() method. Maybe there was a reason why they thought, a BigDecimal with the value "1.000" and "1" should be different, but I can't fully understand it. Fixed, see conversation |
I have the following code snippet that converts 1000 litres to cubic metres: UcumEssenceService ucumEssenceService = new UcumEssenceService(UoMTest.class.getClassLoader().getResourceAsStream("ucum-essence.xml")); BigDecimal mult = ucumEssenceService.convert(new BigDecimal(1000.0), "l", "m3"); Expected result: 1.0 Received result: 100.0 Haven't had much time for deep investigation yet, but looks like exponents are not respected after having basic single dimension components converted. Expected: L = 1 dm^3 = (0.1 m)^3 = 0.001 m^3 What happens as I think: L = 1 dm^3 -> 0.1 (m^3) -> 0.1 m^3