Skip to content

Commit

Permalink
ICU-21349 Remove a potential 0/0 (=NaN). Align C++ & Java better.
Browse files Browse the repository at this point in the history
  • Loading branch information
@hugovdm
hugovdm committed Nov 17, 2020
1 parent a593239 commit 50bd796
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 42 deletions.
22 changes: 9 additions & 13 deletions icu4c/source/i18n/units_complexconverter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,24 +122,20 @@ MaybeStackVector<Measure> 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) {
Expand Down
51 changes: 33 additions & 18 deletions icu4c/source/test/intltest/units_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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();

Expand Down
10 changes: 6 additions & 4 deletions icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,15 +129,17 @@ public List<Measure> convert(BigDecimal quantity, Precision rounder) {
// 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) {
Expand Down
33 changes: 26 additions & 7 deletions icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,25 +64,44 @@ class TestCase {
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),

// 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.

// 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.
// 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),
new Measure(0, MeasureUnit.METER)},
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),
Expand Down

0 comments on commit 50bd796

Please sign in to comment.