ICU-21349 Fix ICU4J reciprocal unit conversions

See #1565

diff --git a/icu4c/source/test/intltest/units_test.cpp b/icu4c/source/test/intltest/units_test.cpp
index e6f66a0abccad64c6d5f006a741d8f248b418332..1ec8062d5334d75f62cbcad729c383dabd2286bb 100644 (file)
--- 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 91c62cb7c908e007e226f5110b5644e24c7aca0e..fea05b9c350312ee9cb2a35532cefdbb26ebfde8 100644 (file)
--- 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-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;   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;   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 4d6d2e0b06fc8d73b256daff6b690ee7ceb8d861..f4f0b385bedc233e2bd6c78b813b3dc5cf421dc5 100644 (file)
--- 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 3d2f9336264950ca111ac339bead8e9692c000d7..fea05b9c350312ee9cb2a35532cefdbb26ebfde8 100644 (file)
--- 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 3a67201ec4e57ad482cdc230ec9b8de6124a92ef..0c03b38b485612eff4efb61472cf91d7c12cc79f 100644 (file)
--- 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<MeasureUnitImpl> outputUnits = new ArrayList<>();
+                for (Pair<String, MeasureUnitImpl> unit : outputUnitInOrder) {
+                    outputUnits.add(unit.second);
+                }
+                return "TestCase: " + category + ", " + usage + ", " + region + "; Input: " + input +
+                    " " + inputUnit.first + "; Expected Values: " + expectedInOrder +
+                    ", Expected Units: " + outputUnits;
             }
         }