From 44c7137ae55a630d591ab37e5ee1a97f0b991ff4 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 3 Dec 2021 14:36:59 +0000 Subject: [PATCH] ICU-21862 icu4c unit conversions: support inverting 0 and Infinity (for vehicle-fuel) See #1947 --- icu4c/source/i18n/units_converter.cpp | 11 ++--- icu4c/source/test/intltest/intltest.cpp | 12 ++++-- icu4c/source/test/intltest/numbertest_api.cpp | 42 ++++++++++++------ icu4c/source/test/intltest/units_test.cpp | 43 ++++++++----------- .../com/ibm/icu/dev/test/impl/UnitsTest.java | 34 ++++++--------- .../test/number/NumberFormatterApiTest.java | 25 ++++++++++- 6 files changed, 96 insertions(+), 71 deletions(-) diff --git a/icu4c/source/i18n/units_converter.cpp b/icu4c/source/i18n/units_converter.cpp index d8ef596d735..82b8eea3d8c 100644 --- 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 7ff28961271..f2956ebcb48 100644 --- 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 c933936b752..8a357436cd1 100644 --- 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 88b677c60e0..ca41e3b0d03 100644 --- 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 1502fe7ecdc..6bfd1a71970 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 @@ -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 ee57c30f5ef..ec2f5f51bd1 100644 --- 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",