ICU-21863 Fix div-by-zero in ICU4J, test inverse unit conversions

Also cleans up some old icu-units TODOs:
- This PR fixes icu-units#38 and icu-units#63 TODOs (now part of ICU-21862)
- icu-units#21 is obsolete

diff --git a/icu4c/source/i18n/units_converter.cpp b/icu4c/source/i18n/units_converter.cpp
index 7e946e584bb76a99db3373fd8cf22a8d29b1da1b..d8ef596d735019244f7278615c50dd14806c0b68 100644 (file)
--- a/icu4c/source/i18n/units_converter.cpp
+++ b/icu4c/source/i18n/units_converter.cpp
@@ -603,9 +603,9 @@ double UnitsConverter::convertInverse(double inputValue) const {
     double result = inputValue;
     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.)
+            // TODO(ICU-21862): demonstrate the resulting behaviour in tests...
+            // and figure out desired behaviour. (Theoretical result should be
+            // infinity, not 0.)
             return 0.0;
         }
         result = 1.0 / result;

diff --git a/icu4c/source/i18n/units_router.h b/icu4c/source/i18n/units_router.h
index b3300f7e27737aa26b0d966fb3feb865da4b26af..d9fcffb2aa9e26ea42ddf70f50df31a051c83a76 100644 (file)
--- a/icu4c/source/i18n/units_router.h
+++ b/icu4c/source/i18n/units_router.h
@@ -30,8 +30,6 @@ namespace units {
 struct RouteResult : UMemory {
     // A list of measures: a single measure for single units, multiple measures
     // for mixed units.
-    //
-    // TODO(icu-units/icu#21): figure out the right mixed unit API.
     MaybeStackVector<Measure> measures;
 
     // The output unit for this RouteResult. This may be a MIXED unit - for

diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp
index cf8fecc0a284ea2e14cab90a2468a4ff863da8ae..c933936b752f82249e130d1556fb39e5d8941ad0 100644 (file)
--- a/icu4c/source/test/intltest/numbertest_api.cpp
+++ b/icu4c/source/test/intltest/numbertest_api.cpp
@@ -1747,6 +1747,28 @@ void NumberFormatterApiTest::unitUsage() {
             30500,
             u"350 m");
 
+    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()                                                  //
+                           .unit(MeasureUnit::forIdentifier("liter-per-100-kilometer", status)) //
+                           .usage("vehicle-fuel"),                                              //
+                       Locale("en-US"),                                                         //
+                       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)
+
     // Test calling `.usage("")` should unset the existing usage.
     // First: without usage
     assertFormatSingle(u"Rounding Mode propagates: rounding up",
@@ -1853,10 +1875,6 @@ void NumberFormatterApiTest::unitUsage() {
                        Locale("en-US"),                                                 //
                        1,                                                               //
                        "0.019 psi");
-
-    // TODO(icu-units#38): improve unit testing coverage. E.g. add vehicle-fuel
-    // triggering inversion conversion code. Test with 0 too, to see
-    // divide-by-zero behaviour.
 }
 
 void NumberFormatterApiTest::unitUsageErrorCodes() {

diff --git a/icu4c/source/test/intltest/units_test.cpp b/icu4c/source/test/intltest/units_test.cpp
index c65a5e03229549f59fa4e3d53f7c1bca0b24f7e0..88b677c60e05e83cc207cd8a9e161cdf5132207d 100644 (file)
--- a/icu4c/source/test/intltest/units_test.cpp
+++ b/icu4c/source/test/intltest/units_test.cpp
@@ -325,6 +325,9 @@ void UnitsTest::testConverter() {
         // 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},
+        {"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},
 
         // Test Aliases
         // Alias is just another name to the same unit. Therefore, converting
@@ -647,6 +650,16 @@ void UnitsTest::testComplexUnitsConverter() {
           Measure(2.1, MeasureUnit::createMeter(status), status)},
          2,
          0.001},
+
+        // Negative numbers
+        {"Negative number conversion",
+         "yard",
+         "mile-and-yard",
+         -1800,
+         {Measure(-1, MeasureUnit::createMile(status), status),
+          Measure(-40, MeasureUnit::createYard(status), status)},
+         2,
+         1e-10},
     };
     status.assertSuccess();
 
@@ -694,11 +707,7 @@ void UnitsTest::testComplexUnitsConverter() {
         ComplexUnitsConverter converter2( testCase.input, testCase.output, status);
         testATestCase(converter2, "ComplexUnitsConverter #1 " , testCase);
     }
-    
-    
     status.assertSuccess();
-
-    // TODO(icu-units#63): test negative numbers!
 }
 
 void UnitsTest::testComplexUnitsConverterSorting() {

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 acece19279e76fc3f10816bfd9d59e75398aef46..156a6e0b6db69ea777d110e63898c611b969b157 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
@@ -115,10 +115,8 @@ public class UnitsConverter {
         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.)
+            if (result.compareTo(BigDecimal.ZERO) == 0) {
+                // TODO(ICU-21862): determine desirable behaviour
                 return BigDecimal.ZERO;
             }
             result = BigDecimal.ONE.divide(result, DECIMAL128);
@@ -131,10 +129,8 @@ public class UnitsConverter {
         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.)
+            if (result.compareTo(BigDecimal.ZERO) == 0) {
+                // TODO(ICU-21862): determine desirable behaviour
                 return BigDecimal.ZERO;
             }
             result = BigDecimal.ONE.divide(result, DECIMAL128);

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 c8706cbbfc4533fef4ee651fccbe7db22c115355..1502fe7ecdc16cfcd944f2fb181cdda32f7d7c2d 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
@@ -90,7 +90,7 @@ public class UnitsTest {
                 0),
 
             // A minimal nudge under 2.0, rounding up to 2.0 ft, 0 in.
-            // TODO(icu-units#108): this matches double precision calculations
+            // TODO(ICU-21861): this matches double precision calculations
             // from C++, but BigDecimal is in use: do we want Java to be more
             // precise than C++?
             new TestCase(
@@ -116,7 +116,7 @@ public class UnitsTest {
             // 1e-16 light years is 0.946073 meters.
 
             // A 2.1 meter nudge under 2.0 light years, rounding up to 2.0 ly, 0 m.
-            // TODO(icu-units#108): this matches double precision calculations
+            // TODO(ICU-21861): this matches double precision calculations
             // from C++, but BigDecimal is in use: do we want Java to be more
             // precise than C++?
             new TestCase("light-year", "light-year-and-meter",
@@ -125,7 +125,7 @@ public class UnitsTest {
                                         new Measure(0, MeasureUnit.METER)},
                          0),
 
-            // // TODO(icu-units#108): figure out precision thresholds for BigDecimal?
+            // // TODO(ICU-21861): figure out precision thresholds for BigDecimal?
             // // This test passes in C++ due to double-precision rounding.
             // // A 2.1 meter nudge under 1.0 light years, rounding up to 1.0 ly, 0 m.
             // new TestCase("light-year", "light-year-and-meter",
@@ -143,7 +143,7 @@ public class UnitsTest {
                                         new Measure(9.46073, MeasureUnit.METER)},
                          0 /* meters, precision */),
 
-            // TODO(icu-units#108): reconsider whether epsilon rounding is desirable:
+            // TODO(ICU-21861): reconsider whether epsilon rounding is desirable:
             //
             // 2e-16 light years is 1.892146 meters. For C++ double, we consider
             // this in the noise, and thus expect a 0. (This test fails when
@@ -153,8 +153,13 @@ public class UnitsTest {
                          new Measure[] {new Measure(1, MeasureUnit.LIGHT_YEAR),
                                         new Measure(1.892146, MeasureUnit.METER)},
                          0),
-        };
 
+            // Negative numbers
+            new TestCase(
+                "yard", "mile-and-yard", BigDecimal.valueOf(-1800),
+                new Measure[] {new Measure(-1, MeasureUnit.MILE), new Measure(-40, MeasureUnit.YARD)},
+                1e-10),
+        };
 
         ConversionRates rates = new ConversionRates();
         MeasureUnit input, output;
@@ -171,8 +176,6 @@ public class UnitsTest {
             ComplexUnitsConverter converter2 = new ComplexUnitsConverter(testCase.input, testCase.output);
             testCase.testATestCase(converter2);
         }
-
-        // TODO(icu-units#63): test negative numbers!
     }
 
 
@@ -468,6 +471,9 @@ public class UnitsTest {
                 // 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),
+                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),
                 // Test Aliases
                 // Alias is just another name to the same unit. Therefore, converting
                 // between them should be the same.

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 c0ab0dadeb3d4af68b0d75705c158560f6277e76..ee57c30f5ef8bb3bb930ce7f83feaac2f760b410 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
@@ -1671,6 +1671,28 @@ public class NumberFormatterApiTest extends TestFmwk {
                 30500,
                 "350 m");
 
+        assertFormatSingle("Fuel consumption: inverted units",
+                           "unit/liter-per-100-kilometer usage/vehicle-fuel",
+                           "unit/liter-per-100-kilometer usage/vehicle-fuel",
+                           NumberFormatter.with()
+                               .unit(MeasureUnit.forIdentifier("liter-per-100-kilometer"))
+                               .usage("vehicle-fuel"),
+                           new ULocale("en-US"), //
+                           6.6,                  //
+                           "36 mpg");
+
+        // // TODO(ICU-21862): determine desired behaviour. Commented out for now
+        // // to not enforce undesirable behaviour
+        // assertFormatSingle("Fuel consumption: inverted units, divide-by-zero",
+        //                    "unit/liter-per-100-kilometer usage/vehicle-fuel",
+        //                    "unit/liter-per-100-kilometer usage/vehicle-fuel",
+        //                    NumberFormatter.with()
+        //                        .unit(MeasureUnit.forIdentifier("liter-per-100-kilometer"))
+        //                        .usage("vehicle-fuel"),
+        //                    new ULocale("en-US"), //
+        //                    0,                    //
+        //                    "0 mpg");
+
         // Test calling .usage("") or .usage(null) should unset the existing usage.
         // First: without usage
         assertFormatSingle("Rounding Mode propagates: rounding up",
@@ -1790,10 +1812,6 @@ public class NumberFormatterApiTest extends TestFmwk {
                 new ULocale("en-US"),
                 1,
                 "0.019 psi");
-
-        // TODO(icu-units#38): improve unit testing coverage. E.g. add
-        // vehicle-fuel triggering inversion conversion code. Test with 0 too,
-        // to see divide-by-zero behaviour.
     }
 
     @Test