ICU-21862 icu4c unit conversions: support inverting 0 and Infinity (for vehicle-fuel)

See #1947

diff --git a/icu4c/source/i18n/units_converter.cpp b/icu4c/source/i18n/units_converter.cpp
index d8ef596d735019244f7278615c50dd14806c0b68..82b8eea3d8cf6cf2f9a48ef277229f0734b22321 100644 (file)
--- a/icu4c/source/i18n/units_converter.cpp
+++ b/icu4c/source/i18n/units_converter.cpp
@@ -9,6 +9,7 @@
 #include "cmemory.h"
 #include "double-conversion-string-to-double.h"
 #include "measunit_impl.h"
+#include "putilimp.h"
 #include "uassert.h"
 #include "unicode/errorcode.h"
 #include "unicode/localpointer.h"
@@ -588,10 +589,7 @@ double UnitsConverter::convert(double inputValue) const {
 
     if (conversionRate_.reciprocal) {
         if (result == 0) {
-            // TODO: demonstrate the resulting behaviour in tests... and figure
-            // out desired behaviour. (Theoretical result should be infinity,
-            // not 0.)
-            return 0.0;
+            return uprv_getInfinity();
         }
         result = 1.0 / result;
     }
@@ -603,10 +601,7 @@ double UnitsConverter::convertInverse(double inputValue) const {
     double result = inputValue;
     if (conversionRate_.reciprocal) {
         if (result == 0) {
-            // TODO(ICU-21862): demonstrate the resulting behaviour in tests...
-            // and figure out desired behaviour. (Theoretical result should be
-            // infinity, not 0.)
-            return 0.0;
+            return uprv_getInfinity();
         }
         result = 1.0 / result;
     }

diff --git a/icu4c/source/test/intltest/intltest.cpp b/icu4c/source/test/intltest/intltest.cpp
index 7ff289612711c5c9e918ae64d82fd21a6a088267..f2956ebcb48952302c67ae99f2fbdbafbc0ee7f4 100644 (file)
--- a/icu4c/source/test/intltest/intltest.cpp
+++ b/icu4c/source/test/intltest/intltest.cpp
@@ -2188,14 +2188,20 @@ UBool IntlTest::assertEqualsNear(const char* message,
                                  double expected,
                                  double actual,
                                  double delta) {
+    bool bothNaN = std::isnan(expected) && std::isnan(actual);
+    bool bothPosInf = uprv_isPositiveInfinity(expected) && uprv_isPositiveInfinity(actual);
+    bool bothNegInf = uprv_isNegativeInfinity(expected) && uprv_isNegativeInfinity(actual);
+    if (bothPosInf || bothNegInf || bothNaN) {
+        // We don't care about delta in these cases
+        return TRUE;
+    }
     if (std::isnan(delta) || std::isinf(delta)) {
         errln((UnicodeString)("FAIL: ") + message + "; nonsensical delta " + delta +
-              " - delta may not be NaN or Inf");
+              " - delta may not be NaN or Inf. (Got " + actual + "; expected " + expected + ".)");
         return FALSE;
     }
-    bool bothNaN = std::isnan(expected) && std::isnan(actual);
     double difference = std::abs(expected - actual);
-    if (expected != actual && (difference > delta || std::isnan(difference)) && !bothNaN) {
+    if (expected != actual && (difference > delta || std::isnan(difference))) {
         errln((UnicodeString)("FAIL: ") + message + "; got " + actual + "; expected " + expected +
               "; acceptable delta " + delta);
         return FALSE;

diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp
index c933936b752f82249e130d1556fb39e5d8941ad0..8a357436cd151a96fe7ec4743770342386a38c85 100644 (file)
--- a/icu4c/source/test/intltest/numbertest_api.cpp
+++ b/icu4c/source/test/intltest/numbertest_api.cpp
@@ -1747,7 +1747,7 @@ void NumberFormatterApiTest::unitUsage() {
             30500,
             u"350 m");
 
-    assertFormatSingle(u"Fuel consumption: inverted units",                                    //
+    assertFormatSingle(u"Fuel consumption: inverted units",                                     //
                        u"unit/liter-per-100-kilometer usage/vehicle-fuel",                      //
                        u"unit/liter-per-100-kilometer usage/vehicle-fuel",                      //
                        NumberFormatter::with()                                                  //
@@ -1757,17 +1757,35 @@ void NumberFormatterApiTest::unitUsage() {
                        6.6,                                                                     //
                        "36 mpg");
 
-//     // TODO(ICU-21862): determine desired behaviour. Commented out for now to not enforce undesirable
-//     // behaviour
-//     assertFormatSingle(u"Fuel consumption: inverted units, divide-by-zero",                     //
-//                        u"unit/liter-per-100-kilometer usage/vehicle-fuel",                      //
-//                        u"unit/liter-per-100-kilometer usage/vehicle-fuel",                      //
-//                        NumberFormatter::with()                                                  //
-//                            .unit(MeasureUnit::forIdentifier("liter-per-100-kilometer", status)) //
-//                            .usage("vehicle-fuel"),                                              //
-//                        Locale("en-US"),                                                         //
-//                        0,                                                                       //
-//                        "0 mpg"); // TODO(ICU-21862)
+    assertFormatSingle(u"Fuel consumption: inverted units, divide-by-zero, en-US",              //
+                       u"unit/liter-per-100-kilometer usage/vehicle-fuel",                      //
+                       u"unit/liter-per-100-kilometer usage/vehicle-fuel",                      //
+                       NumberFormatter::with()                                                  //
+                           .unit(MeasureUnit::forIdentifier("liter-per-100-kilometer", status)) //
+                           .usage("vehicle-fuel"),                                              //
+                       Locale("en-US"),                                                         //
+                       0,                                                                       //
+                       u"∞ mpg");
+
+    assertFormatSingle(u"Fuel consumption: inverted units, divide-by-zero, en-ZA",      //
+                       u"unit/mile-per-gallon usage/vehicle-fuel",                      //
+                       u"unit/mile-per-gallon usage/vehicle-fuel",                      //
+                       NumberFormatter::with()                                          //
+                           .unit(MeasureUnit::forIdentifier("mile-per-gallon", status)) //
+                           .usage("vehicle-fuel"),                                      //
+                       Locale("en-ZA"),                                                 //
+                       0,                                                               //
+                       u"∞ l/100 km");
+
+    assertFormatSingle(u"Fuel consumption: inverted units, divide-by-inf",              //
+                       u"unit/mile-per-gallon usage/vehicle-fuel",                      //
+                       u"unit/mile-per-gallon usage/vehicle-fuel",                      //
+                       NumberFormatter::with()                                          //
+                           .unit(MeasureUnit::forIdentifier("mile-per-gallon", status)) //
+                           .usage("vehicle-fuel"),                                      //
+                       Locale("de-CH"),                                                 //
+                       uprv_getInfinity(),                                              //
+                       u"0 L/100 km");
 
     // Test calling `.usage("")` should unset the existing usage.
     // First: without usage

diff --git a/icu4c/source/test/intltest/units_test.cpp b/icu4c/source/test/intltest/units_test.cpp
index 88b677c60e05e83cc207cd8a9e161cdf5132207d..ca41e3b0d030c242def6ef0a6f13b8b3a7d33490 100644 (file)
--- a/icu4c/source/test/intltest/units_test.cpp
+++ b/icu4c/source/test/intltest/units_test.cpp
@@ -326,8 +326,11 @@ void UnitsTest::testConverter() {
         {"cubic-meter-per-meter", "mile-per-gallon", 2.1383143939394E-6, 1.1},
         {"cubic-meter-per-meter", "mile-per-gallon", 2.6134953703704E-6, 0.9},
         {"liter-per-100-kilometer", "mile-per-gallon", 6.6, 35.6386},
-        // // TODO(ICU-21862): we should probably return something other than "0":
-        // {"liter-per-100-kilometer", "mile-per-gallon", 0, 0},
+        {"liter-per-100-kilometer", "mile-per-gallon", 0, uprv_getInfinity()},
+        {"mile-per-gallon", "liter-per-100-kilometer", 0, uprv_getInfinity()},
+        {"mile-per-gallon", "liter-per-100-kilometer", uprv_getInfinity(), 0},
+        // We skip testing -Inf, because the inverse conversion loses the sign:
+        // {"mile-per-gallon", "liter-per-100-kilometer", -uprv_getInfinity(), 0},
 
         // Test Aliases
         // Alias is just another name to the same unit. Therefore, converting
@@ -351,54 +354,42 @@ void UnitsTest::testConverter() {
             continue;
         }
 
+        double maxDelta = 1e-6 * uprv_fabs(testCase.expectedValue);
+        if (testCase.expectedValue == 0) {
+            maxDelta = 1e-12;
+        }
+        double inverseMaxDelta = 1e-6 * uprv_fabs(testCase.inputValue);
+        if (testCase.inputValue == 0) {
+            inverseMaxDelta = 1e-12;
+        }
+
         ConversionRates conversionRates(status);
         if (status.errIfFailureAndReset("conversionRates(status)")) {
             continue;
         }
+
         UnitsConverter converter(source, target, conversionRates, status);
         if (status.errIfFailureAndReset("UnitsConverter(<%s>, <%s>, ...)", testCase.source,
                                         testCase.target)) {
             continue;
         }
-
-        double maxDelta = 1e-6 * uprv_fabs(testCase.expectedValue);
-        if (testCase.expectedValue == 0) {
-            maxDelta = 1e-12;
-        }
         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);
+            testCase.inputValue, converter.convertInverse(testCase.expectedValue), inverseMaxDelta);
 
-
-        // TODO: Test UnitsConverter created using CLDR separately.
         // Test UnitsConverter created by CLDR unit identifiers
         UnitsConverter converter2(testCase.source, testCase.target, status);
         if (status.errIfFailureAndReset("UnitsConverter(<%s>, <%s>, ...)", testCase.source,
                                         testCase.target)) {
             continue;
         }
-
-        maxDelta = 1e-6 * uprv_fabs(testCase.expectedValue);
-        if (testCase.expectedValue == 0) {
-            maxDelta = 1e-12;
-        }
         assertEqualsNear(UnicodeString("testConverter2: ") + testCase.source + " to " + testCase.target,
                          testCase.expectedValue, converter2.convert(testCase.inputValue), maxDelta);
-
-        maxDelta = 1e-6 * uprv_fabs(testCase.inputValue);
-        if (testCase.inputValue == 0) {
-            maxDelta = 1e-12;
-        }
         assertEqualsNear(
             UnicodeString("testConverter2 inverse: ") + testCase.target + " back to " + testCase.source,
-            testCase.inputValue, converter2.convertInverse(testCase.expectedValue), maxDelta);
+            testCase.inputValue, converter2.convertInverse(testCase.expectedValue), inverseMaxDelta);
     }
 }
 

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 1502fe7ecdc16cfcd944f2fb181cdda32f7d7c2d..6bfd1a71970114d54ed998e1c944e9aafe2322cb 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
@@ -474,6 +474,11 @@ public class UnitsTest {
                 new TestData("liter-per-100-kilometer", "mile-per-gallon", 6.6, 35.6386),
                 // // TODO(ICU-21862): we should probably return something other than "0":
                 // new TestData("liter-per-100-kilometer", "mile-per-gallon", 0, 0),
+                // new TestData("mile-per-gallon", "liter-per-100-kilometer", 0, 0),
+                // // TODO(ICU-21862): deal with infinity input in Java?
+                // new TestData("mile-per-gallon", "liter-per-100-kilometer", INFINITY, 0),
+                // We skip testing -Inf, because the inverse conversion loses the sign:
+                // new TestData("mile-per-gallon", "liter-per-100-kilometer", -INFINITY, 0),
                 // Test Aliases
                 // Alias is just another name to the same unit. Therefore, converting
                 // between them should be the same.
@@ -488,45 +493,32 @@ public class UnitsTest {
             MeasureUnitImpl source = MeasureUnitImpl.forIdentifier(test.sourceIdentifier);
             MeasureUnitImpl target = MeasureUnitImpl.forIdentifier(test.targetIdentifier);
 
-            UnitsConverter converter = new UnitsConverter(source, target, conversionRates);
-
             double maxDelta = 1e-6 * Math.abs(test.expected.doubleValue());
             if (test.expected.doubleValue() == 0) {
                 maxDelta = 1e-12;
             }
+            double inverseMaxDelta = 1e-6 * Math.abs(test.input.doubleValue());
+            if (test.input.doubleValue() == 0) {
+                inverseMaxDelta = 1e-12;
+            }
+
+            UnitsConverter converter = new UnitsConverter(source, target, conversionRates);
             assertEquals("testConverter: " + test.sourceIdentifier + " to " + test.targetIdentifier,
                     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.targetIdentifier + " back to " + test.sourceIdentifier,
                     test.input.doubleValue(), converter.convertInverse(test.expected).doubleValue(),
-                    maxDelta);
+                    inverseMaxDelta);
 
-
-            // TODO: Test UnitsConverter created using CLDR separately.
             // Test UnitsConverter created by CLDR unit identifiers
             UnitsConverter converter2 = new UnitsConverter(test.sourceIdentifier, test.targetIdentifier);
-
-            maxDelta = 1e-6 * Math.abs(test.expected.doubleValue());
-            if (test.expected.doubleValue() == 0) {
-                maxDelta = 1e-12;
-            }
             assertEquals("testConverter2: " + test.sourceIdentifier + " to " + test.targetIdentifier,
                     test.expected.doubleValue(), converter2.convert(test.input).doubleValue(),
                     maxDelta);
-
-            maxDelta = 1e-6 * Math.abs(test.input.doubleValue());
-            if (test.input.doubleValue() == 0) {
-                maxDelta = 1e-12;
-            }
             assertEquals("testConverter2 inverse: " + test.targetIdentifier + " back to " + test.sourceIdentifier,
                     test.input.doubleValue(), converter2.convertInverse(test.expected).doubleValue(),
-                    maxDelta);
+                    inverseMaxDelta);
         }
     }
 

diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
index ee57c30f5ef8bb3bb930ce7f83feaac2f760b410..ec2f5f51bd1dd386f0b235d8e72c8160da601a5c 100644 (file)
--- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
+++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
@@ -1683,7 +1683,7 @@ public class NumberFormatterApiTest extends TestFmwk {
 
         // // TODO(ICU-21862): determine desired behaviour. Commented out for now
         // // to not enforce undesirable behaviour
-        // assertFormatSingle("Fuel consumption: inverted units, divide-by-zero",
+        // assertFormatSingle("Fuel consumption: inverted units, divide-by-zero, en-US",
         //                    "unit/liter-per-100-kilometer usage/vehicle-fuel",
         //                    "unit/liter-per-100-kilometer usage/vehicle-fuel",
         //                    NumberFormatter.with()
@@ -1693,6 +1693,29 @@ public class NumberFormatterApiTest extends TestFmwk {
         //                    0,                    //
         //                    "0 mpg");
 
+        // // TODO(ICU-21862): determine desired behaviour. Commented out for now
+        // // to not enforce undesirable behaviour
+        // assertFormatSingle("Fuel consumption: inverted units, divide-by-zero, en-ZA",
+        //                    "unit/mile-per-gallon usage/vehicle-fuel",
+        //                    "unit/mile-per-gallon usage/vehicle-fuel",
+        //                    NumberFormatter.with()
+        //                        .unit(MeasureUnit.forIdentifier("mile-per-gallon"))
+        //                        .usage("vehicle-fuel"),
+        //                    new ULocale("en-ZA"), //
+        //                    0,                    //
+        //                    "0 mpg");
+
+        // // TODO(ICU-21862): Once we support Inf as input:
+        // assertFormatSingle("Fuel consumption: inverted units, divide-by-inf",
+        //                    "unit/mile-per-gallon usage/vehicle-fuel",
+        //                    "unit/mile-per-gallon usage/vehicle-fuel",
+        //                    NumberFormatter.with()
+        //                        .unit(MeasureUnit.forIdentifier("mile-per-gallon"))
+        //                        .usage("vehicle-fuel"),
+        //                    new ULocale("de-CH"), //
+        //                    INFINITY_GOES_HERE,   //
+        //                    "0 mpg");
+
         // Test calling .usage("") or .usage(null) should unset the existing usage.
         // First: without usage
         assertFormatSingle("Rounding Mode propagates: rounding up",