From 377dc22280677b12a4a255b309a2c9d05b4fb0a5 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 6 Feb 2021 00:06:33 +0000 Subject: [PATCH] ICU-21349 Fix ICU4J reciprocal unit conversions See #1565 --- icu4c/source/test/intltest/units_test.cpp | 13 ++++++- .../cldr/units/unitPreferencesTest.txt | 28 ++++----------- .../ibm/icu/impl/units/UnitsConverter.java | 35 +++++++++++++++++-- .../data/cldr/units/unitPreferencesTest.txt | 20 ++++------- .../com/ibm/icu/dev/test/impl/UnitsTest.java | 21 +++++++++-- 5 files changed, 76 insertions(+), 41 deletions(-) diff --git a/icu4c/source/test/intltest/units_test.cpp b/icu4c/source/test/intltest/units_test.cpp index e6f66a0abcc..1ec8062d533 100644 --- a/icu4c/source/test/intltest/units_test.cpp +++ b/icu4c/source/test/intltest/units_test.cpp @@ -198,7 +198,7 @@ void UnitsTest::testConverter() { {"mebibyte", "byte", 1, 1048576}, {"gibibyte", "kibibyte", 1, 1048576}, {"pebibyte", "tebibyte", 4, 4096}, - {"zebibyte", "pebibyte", 1.0/16, 65536.0}, + {"zebibyte", "pebibyte", 1.0 / 16, 65536.0}, {"yobibyte", "exbibyte", 1, 1048576}, // Mass {"gram", "kilogram", 1.0, 0.001}, @@ -232,6 +232,9 @@ void UnitsTest::testConverter() { {"square-yard", "square-foot", 0, 0}, {"square-yard", "square-foot", 0.000001, 0.000009}, {"square-mile", "square-foot", 0.0, 0.0}, + // Fuel Consumption + {"cubic-meter-per-meter", "mile-per-gallon", 2.1383143939394E-6, 1.1}, + {"cubic-meter-per-meter", "mile-per-gallon", 2.6134953703704E-6, 0.9}, }; for (const auto &testCase : testCases) { @@ -262,6 +265,14 @@ void UnitsTest::testConverter() { } assertEqualsNear(UnicodeString("testConverter: ") + testCase.source + " to " + testCase.target, testCase.expectedValue, converter.convert(testCase.inputValue), maxDelta); + + maxDelta = 1e-6 * uprv_fabs(testCase.inputValue); + if (testCase.inputValue == 0) { + maxDelta = 1e-12; + } + assertEqualsNear( + UnicodeString("testConverter inverse: ") + testCase.target + " back to " + testCase.source, + testCase.inputValue, converter.convertInverse(testCase.expectedValue), maxDelta); } } diff --git a/icu4c/source/test/testdata/cldr/units/unitPreferencesTest.txt b/icu4c/source/test/testdata/cldr/units/unitPreferencesTest.txt index 91c62cb7c90..fea05b9c350 100644 --- a/icu4c/source/test/testdata/cldr/units/unitPreferencesTest.txt +++ b/icu4c/source/test/testdata/cldr/units/unitPreferencesTest.txt @@ -83,29 +83,13 @@ consumption; vehicle-fuel; BR; 11 / 10000000; 1.1E-6; cubic-meter-per-meter; 11 consumption; vehicle-fuel; BR; 1 / 1000000; 1.0E-6; cubic-meter-per-meter; 1; 1.0; liter-per-kilometer consumption; vehicle-fuel; BR; 9 / 10000000; 9.0E-7; cubic-meter-per-meter; 9 / 10; 0.9; liter-per-kilometer -# TODO: these are local ICU-specific edits until the CLDR test file is updated: -# consumption-inverse; default; 001; 110000000; 1.1E8; meter-per-cubic-meter; 11 / 10; 1.1; kilometer-per-centiliter -consumption; default; 001; 110000000; 1.1E8; meter-per-cubic-meter; 10 / 11; 0.90909090909090906; liter-per-100-kilometer -# consumption-inverse; default; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; kilometer-per-centiliter -consumption; default; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; liter-per-100-kilometer -# consumption-inverse; default; 001; 90000000; 9.0E7; meter-per-cubic-meter; 9 / 10; 0.9; kilometer-per-centiliter -consumption; default; 001; 90000000; 9.0E7; meter-per-cubic-meter; 10 / 9; 1.1111111111111112; liter-per-100-kilometer +consumption; vehicle-fuel; US; 112903 / 52800000000; 2.1383143939394E-6; cubic-meter-per-meter; 11 / 10; 1.1; mile-per-gallon +consumption; vehicle-fuel; US; 112903 / 48000000000; 2.3521458333333E-6; cubic-meter-per-meter; 1; 1.0; mile-per-gallon +consumption; vehicle-fuel; US; 112903 / 43200000000; 2.6134953703704E-6; cubic-meter-per-meter; 9 / 10; 0.9; mile-per-gallon -# consumption-inverse; vehicle-fuel; 001; 110000000; 1.1E8; meter-per-cubic-meter; 11 / 10; 1.1; kilometer-per-centiliter -consumption; vehicle-fuel; 001; 110000000; 1.1E8; meter-per-cubic-meter; 10 / 11; 0.90909090909090906; liter-per-100-kilometer -# consumption-inverse; vehicle-fuel; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; kilometer-per-centiliter -consumption; vehicle-fuel; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; liter-per-100-kilometer -# consumption-inverse; vehicle-fuel; 001; 90000000; 9.0E7; meter-per-cubic-meter; 9 / 10; 0.9; kilometer-per-centiliter -consumption; vehicle-fuel; 001; 90000000; 9.0E7; meter-per-cubic-meter; 10 / 9; 1.1111111111111112; liter-per-100-kilometer - -# TODO: these still pass, as expected/desired. Leaving them categorised as consumption-inverse, this is ignored by tests anyway -consumption-inverse; vehicle-fuel; US; 52800000000 / 112903; 467658.0781732992; meter-per-cubic-meter; 11 / 10; 1.1; mile-per-gallon -consumption-inverse; vehicle-fuel; US; 48000000000 / 112903; 425143.707430272; meter-per-cubic-meter; 1; 1.0; mile-per-gallon -consumption-inverse; vehicle-fuel; US; 43200000000 / 112903; 382629.3366872448; meter-per-cubic-meter; 9 / 10; 0.9; mile-per-gallon - -consumption-inverse; vehicle-fuel; CA; 177027840000 / 454609; 389406.8089281118; meter-per-cubic-meter; 11 / 10; 1.1; mile-per-gallon-imperial -consumption-inverse; vehicle-fuel; CA; 160934400000 / 454609; 354006.1899346471; meter-per-cubic-meter; 1; 1.0; mile-per-gallon-imperial -consumption-inverse; vehicle-fuel; CA; 144840960000 / 454609; 318605.5709411824; meter-per-cubic-meter; 9 / 10; 0.9; mile-per-gallon-imperial +consumption; vehicle-fuel; CA; 454609 / 177027840000; 2.5680085121075E-6; cubic-meter-per-meter; 11 / 10; 1.1; mile-per-gallon-imperial +consumption; vehicle-fuel; CA; 454609 / 160934400000; 2.8248093633182E-6; cubic-meter-per-meter; 1; 1.0; mile-per-gallon-imperial +consumption; vehicle-fuel; CA; 454609 / 144840960000; 3.1386770703536E-6; cubic-meter-per-meter; 9 / 10; 0.9; mile-per-gallon-imperial duration; default; 001; 95040; 95040.0; second; 11 / 10; 1.1; day duration; default; 001; 86400; 86400.0; second; 1; 1.0; day diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java index 4d6d2e0b06f..f4f0b385bed 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java @@ -13,6 +13,7 @@ import com.ibm.icu.util.MeasureUnit; public class UnitsConverter { private BigDecimal conversionRate; + private boolean reciprocal; private BigDecimal offset; /** @@ -27,6 +28,7 @@ public class UnitsConverter { */ public UnitsConverter(MeasureUnitImpl source, MeasureUnitImpl target, ConversionRates conversionRates) { Convertibility convertibility = extractConvertibility(source, target, conversionRates); + // TODO(icu-units#82): throw exception if conversion between incompatible types was requested? assert (convertibility == Convertibility.CONVERTIBLE || convertibility == Convertibility.RECIPROCAL); Factor sourceToBase = conversionRates.getFactorToBase(source); @@ -35,11 +37,15 @@ public class UnitsConverter { if (convertibility == Convertibility.CONVERTIBLE) { this.conversionRate = sourceToBase.divide(targetToBase).getConversionRate(); } else { + assert convertibility == Convertibility.RECIPROCAL; this.conversionRate = sourceToBase.multiply(targetToBase).getConversionRate(); } + this.reciprocal = convertibility == Convertibility.RECIPROCAL; // calculate the offset this.offset = conversionRates.getOffset(source, target, sourceToBase, targetToBase, convertibility); + // We should see no offsets for reciprocal conversions - they don't make sense: + assert convertibility != Convertibility.RECIPROCAL || this.offset == BigDecimal.ZERO; } static public Convertibility extractConvertibility(MeasureUnitImpl source, MeasureUnitImpl target, ConversionRates conversionRates) { @@ -83,11 +89,36 @@ public class UnitsConverter { } public BigDecimal convert(BigDecimal inputValue) { - return inputValue.multiply(this.conversionRate).add(offset); + BigDecimal result = inputValue.multiply(this.conversionRate).add(offset); + if (this.reciprocal) { + // We should see no offsets for reciprocal conversions - they don't make sense: + assert offset == BigDecimal.ZERO; + if (result == BigDecimal.ZERO) { + // TODO: demonstrate the resulting behaviour in tests... and + // figure out desired behaviour. (Theoretical result should be + // infinity, not 0, but BigDecimal does not support infinity.) + return BigDecimal.ZERO; + } + result = BigDecimal.ONE.divide(result, DECIMAL128); + } + return result; } public BigDecimal convertInverse(BigDecimal inputValue) { - return inputValue.subtract(offset).divide(this.conversionRate, DECIMAL128); + BigDecimal result = inputValue; + if (this.reciprocal) { + // We should see no offsets for reciprocal conversions - they don't make sense: + assert offset == BigDecimal.ZERO; + if (result == BigDecimal.ZERO) { + // TODO: demonstrate the resulting behaviour in tests... and + // figure out desired behaviour. (Theoretical result should be + // infinity, not 0, but BigDecimal does not support infinity.) + return BigDecimal.ZERO; + } + result = BigDecimal.ONE.divide(result, DECIMAL128); + } + result = result.subtract(offset).divide(this.conversionRate, DECIMAL128); + return result; } public enum Convertibility { diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/data/cldr/units/unitPreferencesTest.txt b/icu4j/main/tests/core/src/com/ibm/icu/dev/data/cldr/units/unitPreferencesTest.txt index 3d2f9336264..fea05b9c350 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/data/cldr/units/unitPreferencesTest.txt +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/data/cldr/units/unitPreferencesTest.txt @@ -83,21 +83,13 @@ consumption; vehicle-fuel; BR; 11 / 10000000; 1.1E-6; cubic-meter-per-meter; 11 consumption; vehicle-fuel; BR; 1 / 1000000; 1.0E-6; cubic-meter-per-meter; 1; 1.0; liter-per-kilometer consumption; vehicle-fuel; BR; 9 / 10000000; 9.0E-7; cubic-meter-per-meter; 9 / 10; 0.9; liter-per-kilometer -consumption-inverse; default; 001; 110000000; 1.1E8; meter-per-cubic-meter; 11 / 10; 1.1; kilometer-per-centiliter -consumption-inverse; default; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; kilometer-per-centiliter -consumption-inverse; default; 001; 90000000; 9.0E7; meter-per-cubic-meter; 9 / 10; 0.9; kilometer-per-centiliter +consumption; vehicle-fuel; US; 112903 / 52800000000; 2.1383143939394E-6; cubic-meter-per-meter; 11 / 10; 1.1; mile-per-gallon +consumption; vehicle-fuel; US; 112903 / 48000000000; 2.3521458333333E-6; cubic-meter-per-meter; 1; 1.0; mile-per-gallon +consumption; vehicle-fuel; US; 112903 / 43200000000; 2.6134953703704E-6; cubic-meter-per-meter; 9 / 10; 0.9; mile-per-gallon -consumption-inverse; vehicle-fuel; 001; 110000000; 1.1E8; meter-per-cubic-meter; 11 / 10; 1.1; kilometer-per-centiliter -consumption-inverse; vehicle-fuel; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; kilometer-per-centiliter -consumption-inverse; vehicle-fuel; 001; 90000000; 9.0E7; meter-per-cubic-meter; 9 / 10; 0.9; kilometer-per-centiliter - -consumption-inverse; vehicle-fuel; US; 52800000000 / 112903; 467658.0781732992; meter-per-cubic-meter; 11 / 10; 1.1; mile-per-gallon -consumption-inverse; vehicle-fuel; US; 48000000000 / 112903; 425143.707430272; meter-per-cubic-meter; 1; 1.0; mile-per-gallon -consumption-inverse; vehicle-fuel; US; 43200000000 / 112903; 382629.3366872448; meter-per-cubic-meter; 9 / 10; 0.9; mile-per-gallon - -consumption-inverse; vehicle-fuel; CA; 177027840000 / 454609; 389406.8089281118; meter-per-cubic-meter; 11 / 10; 1.1; mile-per-gallon-imperial -consumption-inverse; vehicle-fuel; CA; 160934400000 / 454609; 354006.1899346471; meter-per-cubic-meter; 1; 1.0; mile-per-gallon-imperial -consumption-inverse; vehicle-fuel; CA; 144840960000 / 454609; 318605.5709411824; meter-per-cubic-meter; 9 / 10; 0.9; mile-per-gallon-imperial +consumption; vehicle-fuel; CA; 454609 / 177027840000; 2.5680085121075E-6; cubic-meter-per-meter; 11 / 10; 1.1; mile-per-gallon-imperial +consumption; vehicle-fuel; CA; 454609 / 160934400000; 2.8248093633182E-6; cubic-meter-per-meter; 1; 1.0; mile-per-gallon-imperial +consumption; vehicle-fuel; CA; 454609 / 144840960000; 3.1386770703536E-6; cubic-meter-per-meter; 9 / 10; 0.9; mile-per-gallon-imperial duration; default; 001; 95040; 95040.0; second; 11 / 10; 1.1; day duration; default; 001; 86400; 86400.0; second; 1; 1.0; day diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java index 3a67201ec4e..0c03b38b485 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java @@ -336,11 +336,15 @@ public class UnitsTest { new TestData("square-yard", "square-foot", 0, 0), new TestData("square-yard", "square-foot", 0.000001, 0.000009), new TestData("square-mile", "square-foot", 0.0, 0.0), + // Fuel Consumption + new TestData("cubic-meter-per-meter", "mile-per-gallon", 2.1383143939394E-6, 1.1), + new TestData("cubic-meter-per-meter", "mile-per-gallon", 2.6134953703704E-6, 0.9), }; ConversionRates conversionRates = new ConversionRates(); for (TestData test : tests) { UnitsConverter converter = new UnitsConverter(test.source, test.target, conversionRates); + double maxDelta = 1e-6 * Math.abs(test.expected.doubleValue()); if (test.expected.doubleValue() == 0) { maxDelta = 1e-12; @@ -348,6 +352,14 @@ public class UnitsTest { assertEquals("testConverter: " + test.source + " to " + test.target, test.expected.doubleValue(), converter.convert(test.input).doubleValue(), maxDelta); + + maxDelta = 1e-6 * Math.abs(test.input.doubleValue()); + if (test.input.doubleValue() == 0) { + maxDelta = 1e-12; + } + assertEquals("testConverter inverse: " + test.target + " back to " + test.source, + test.input.doubleValue(), converter.convertInverse(test.expected).doubleValue(), + maxDelta); } } @@ -489,8 +501,13 @@ public class UnitsTest { } public String toString() { - return "TestCase: " + category + ", " + usage + ", " + region + - "; Input: " + input + " " + inputUnit.first + "; Expected Values: " + expectedInOrder; + ArrayList outputUnits = new ArrayList<>(); + for (Pair unit : outputUnitInOrder) { + outputUnits.add(unit.second); + } + return "TestCase: " + category + ", " + usage + ", " + region + "; Input: " + input + + " " + inputUnit.first + "; Expected Values: " + expectedInOrder + + ", Expected Units: " + outputUnits; } }