diff --git a/icu4c/source/i18n/units_complexconverter.cpp b/icu4c/source/i18n/units_complexconverter.cpp index 645c9c01cd5..7abe89ff722 100644 --- a/icu4c/source/i18n/units_complexconverter.cpp +++ b/icu4c/source/i18n/units_complexconverter.cpp @@ -122,24 +122,20 @@ MaybeStackVector ComplexUnitsConverter::convert(double quantity, for (int i = 0, n = unitConverters_.length(); i < n; ++i) { quantity = (*unitConverters_[i]).convert(quantity); if (i < n - 1) { - // The double type has 15 decimal digits of precision. For choosing - // whether to use the current unit or the next smaller unit, we - // therefore nudge up the number with which the thresholding - // decision is made. However after the thresholding, we use the - // original values to ensure unbiased accuracy (to the extent of - // double's capabilities). - int64_t roundedQuantity = floor(quantity * (1 + DBL_EPSILON)); - intValues[i] = roundedQuantity; + // If quantity is at the limits of double's precision from an + // integer value, we take that integer value. + int64_t flooredQuantity = floor(quantity * (1 + DBL_EPSILON)); + intValues[i] = flooredQuantity; // Keep the residual of the quantity. // For example: `3.6 feet`, keep only `0.6 feet` - // - // When the calculation is near enough +/- DBL_EPSILON, we round to - // zero. (We also ensure no negative values here.) - if ((quantity - roundedQuantity) / quantity < DBL_EPSILON) { + double remainder = quantity - flooredQuantity; + if (remainder < 0) { + // Because we nudged flooredQuantity up by eps, remainder may be + // negative: we must treat such a remainder as zero. quantity = 0; } else { - quantity -= roundedQuantity; + quantity = remainder; } } else { // LAST ELEMENT if (rounder == nullptr) { diff --git a/icu4c/source/test/intltest/units_test.cpp b/icu4c/source/test/intltest/units_test.cpp index c88116ae3ce..6eea5ec2676 100644 --- a/icu4c/source/test/intltest/units_test.cpp +++ b/icu4c/source/test/intltest/units_test.cpp @@ -405,6 +405,13 @@ void UnitsTest::testConverterWithCLDRTests() { void UnitsTest::testComplexUnitsConverter() { IcuTestErrorCode status(*this, "UnitsTest::testComplexUnitsConverter"); + // DBL_EPSILON is aproximately 2.22E-16, and is the precision of double for + // values in the range [1.0, 2.0), but half the precision of double for + // [2.0, 4.0). + U_ASSERT(1.0 + DBL_EPSILON > 1.0); + U_ASSERT(2.0 - DBL_EPSILON < 2.0); + U_ASSERT(2.0 + DBL_EPSILON == 2.0); + struct TestCase { const char* msg; const char* input; @@ -425,7 +432,6 @@ void UnitsTest::testComplexUnitsConverter() { 2, 0}, - // TODO(icu-units#108): reconsider whether desireable to round up: // A minimal nudge under 2.0, rounding up to 2.0 ft, 0 in. {"2-eps", "foot", @@ -436,12 +442,27 @@ void UnitsTest::testComplexUnitsConverter() { 2, 0}, - // Testing precision with meter and light-year. 1e-16 light years is - // 0.946073 meters, and double precision can provide only ~15 decimal - // digits, so we don't expect to get anything less than 1 meter. + // A slightly bigger nudge under 2.0, *not* rounding up to 2.0 ft! + {"2-3eps", + "foot", + "foot-and-inch", + 2.0 - 3 * DBL_EPSILON, + {Measure(1, MeasureUnit::createFoot(status), status), + // We expect 12*3*DBL_EPSILON inches (7.92e-15) less than 12. + Measure(12 - 36 * DBL_EPSILON, MeasureUnit::createInch(status), status)}, + 2, + // Might accuracy be lacking with some compilers or on some systems? In + // case it is somehow lacking, we'll allow a delta of 12 * DBL_EPSILON. + 12 * DBL_EPSILON}, - // TODO(icu-units#108): reconsider whether desireable to round up: - // A nudge under 2.0 light years, rounding up to 2.0 ly, 0 m. + // Testing precision with meter and light-year. + // + // DBL_EPSILON light-years, ~2.22E-16 light-years, is ~2.1 meters + // (maximum precision when exponent is 0). + // + // 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. {"2-eps", "light-year", "light-year-and-meter", @@ -451,8 +472,7 @@ void UnitsTest::testComplexUnitsConverter() { 2, 0}, - // TODO(icu-units#108): reconsider whether desireable to round up: - // A nudge under 1.0 light years, rounding up to 1.0 ly, 0 m. + // A 2.1 meter nudge under 1.0 light years, rounding up to 1.0 ly, 0 m. {"1-eps", "light-year", "light-year-and-meter", @@ -475,20 +495,15 @@ void UnitsTest::testComplexUnitsConverter() { 2, 1.5 /* meters, precision */}, - // TODO(icu-units#108): 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 - // 2e-16 is increased to 4e-16.) For Java, using BigDecimal, we - // actually get a good result. - {"1 + 2e-16", + // 2.1 meters more than 1 light year is not rounded away. + {"1 + eps", "light-year", "light-year-and-meter", - 1.0 + 2e-16, + 1.0 + DBL_EPSILON, {Measure(1, MeasureUnit::createLightYear(status), status), - Measure(0, MeasureUnit::createMeter(status), status)}, + Measure(2.1, MeasureUnit::createMeter(status), status)}, 2, - 0}, + 0.001}, }; status.assertSuccess(); diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java index 96e88100f53..dbf5b6d28a0 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java @@ -129,15 +129,17 @@ public class ComplexUnitsConverter { // decision is made. However after the thresholding, we use the // original values to ensure unbiased accuracy (to the extent of // double's capabilities). - BigDecimal roundedQuantity = + BigDecimal flooredQuantity = quantity.multiply(EPSILON_MULTIPLIER).setScale(0, RoundingMode.FLOOR); - intValues.add(roundedQuantity); + intValues.add(flooredQuantity); // Keep the residual of the quantity. // For example: `3.6 feet`, keep only `0.6 feet` - quantity = quantity.subtract(roundedQuantity); - if (quantity.compareTo(BigDecimal.ZERO) == -1) { + BigDecimal remainder = quantity.subtract(flooredQuantity); + if (remainder.compareTo(BigDecimal.ZERO) == -1) { quantity = BigDecimal.ZERO; + } else { + quantity = remainder; } } else { // LAST ELEMENT if (rounder == null) { 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 554ed6b9639..b3f38e2d6d6 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 @@ -64,16 +64,35 @@ 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 + // from C++, but BigDecimal is in use: do we want Java to be more + // precise than C++? new TestCase( "foot", "foot-and-inch", BigDecimal.valueOf(2.0).subtract(ComplexUnitsConverter.EPSILON), new Measure[] {new Measure(2, MeasureUnit.FOOT), new Measure(0, MeasureUnit.INCH)}, 0), - // Testing precision with meter and light-year. 1e-16 light years is - // 0.946073 meters, and double precision can provide only ~15 decimal - // digits, so we don't expect to get anything less than 1 meter. + // A slightly bigger nudge under 2.0, *not* rounding up to 2.0 ft! + new TestCase("foot", "foot-and-inch", + BigDecimal.valueOf(2.0).subtract( + ComplexUnitsConverter.EPSILON.multiply(BigDecimal.valueOf(3.0))), + new Measure[] {new Measure(1, MeasureUnit.FOOT), + new Measure(BigDecimal.valueOf(12.0).subtract( + ComplexUnitsConverter.EPSILON.multiply( + BigDecimal.valueOf(36.0))), + MeasureUnit.INCH)}, + 0), - // TODO(icu-units#108): figure out precision thresholds for BigDecimal? - // A nudge under 2.0 light years, rounding up to 2.0 ly, 0 m. + // Testing precision with meter and light-year. + // + // DBL_EPSILON light-years, ~2.22E-16 light-years, is ~2.1 meters + // (maximum precision when exponent is 0). + // + // 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 + // 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", BigDecimal.valueOf(2.0).subtract(ComplexUnitsConverter.EPSILON), new Measure[] {new Measure(2, MeasureUnit.LIGHT_YEAR), @@ -81,8 +100,8 @@ public class UnitsTest { 0), // // TODO(icu-units#108): figure out precision thresholds for BigDecimal? - // // This test is passing in C++ but failing in Java: - // // A nudge under 1.0 light years, rounding up to 1.0 ly, 0 m. + // // 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", // BigDecimal.valueOf(1.0).subtract(ComplexUnitsConverter.EPSILON), // new Measure[] {new Measure(1, MeasureUnit.LIGHT_YEAR),