diff --git a/icu4c/source/i18n/number_rounding.cpp b/icu4c/source/i18n/number_rounding.cpp index 14896679696..a8fd6bc892a 100644 --- a/icu4c/source/i18n/number_rounding.cpp +++ b/icu4c/source/i18n/number_rounding.cpp @@ -24,10 +24,9 @@ using namespace icu::number::impl; using double_conversion::DoubleToStringConverter; using icu::StringSegment; -// Most blueprint_helpers live in number_skeletons.cpp. This one is in -// number_rounding.cpp for dependency reasons. -void blueprint_helpers::parseIncrementOption(const StringSegment &segment, MacroProps ¯os, - UErrorCode &status) { +void number::impl::parseIncrementOption(const StringSegment &segment, + Precision &outPrecision, + UErrorCode &status) { // Need to do char <-> UChar conversion... U_ASSERT(U_SUCCESS(status)); CharString buffer; @@ -50,10 +49,10 @@ void blueprint_helpers::parseIncrementOption(const StringSegment &segment, Macro decimalOffset++; } if (decimalOffset == segment.length()) { - macros.precision = Precision::increment(increment); + outPrecision = Precision::increment(increment); } else { int32_t fractionLength = segment.length() - decimalOffset - 1; - macros.precision = Precision::increment(increment).withMinFraction(fractionLength); + outPrecision = Precision::increment(increment).withMinFraction(fractionLength); } } diff --git a/icu4c/source/i18n/number_roundingutils.h b/icu4c/source/i18n/number_roundingutils.h index c6e5c77d8c8..e85cbae9fdd 100644 --- a/icu4c/source/i18n/number_roundingutils.h +++ b/icu4c/source/i18n/number_roundingutils.h @@ -8,6 +8,7 @@ #define __NUMBER_ROUNDINGUTILS_H__ #include "number_types.h" +#include "string_segment.h" U_NAMESPACE_BEGIN namespace number { @@ -192,12 +193,20 @@ class RoundingImpl { bool fPassThrough = true; // default value // Permits access to fPrecision. - friend class UsagePrefsHandler; + friend class units::UnitsRouter; // Permits access to fPrecision. friend class UnitConversionHandler; }; +/** + * Parses Precision-related skeleton strings without knowledge of MacroProps + * - see blueprint_helpers::parseIncrementOption(). + * + * Referencing MacroProps means needing to pull in the .o files that have the + * destructors for the SymbolsWrapper, Usage, and Scale classes. + */ +void parseIncrementOption(const StringSegment &segment, Precision &outPrecision, UErrorCode &status); } // namespace impl } // namespace number diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp index 5a23bd32146..fd7d7ca1a1a 100644 --- a/icu4c/source/i18n/number_skeletons.cpp +++ b/icu4c/source/i18n/number_skeletons.cpp @@ -10,6 +10,7 @@ #define UNISTR_FROM_STRING_EXPLICIT #include "number_decnum.h" +#include "number_roundingutils.h" #include "number_skeletons.h" #include "umutex.h" #include "ucln_in.h" @@ -1333,8 +1334,10 @@ bool blueprint_helpers::parseFracSigOption(const StringSegment& segment, MacroPr return true; } -// blueprint_helpers::parseIncrementOption lives in number_rounding.cpp for -// dependencies reasons. +void blueprint_helpers::parseIncrementOption(const StringSegment &segment, MacroProps ¯os, + UErrorCode &status) { + number::impl::parseIncrementOption(segment, macros.precision, status); +} void blueprint_helpers::generateIncrementOption(double increment, int32_t trailingZeros, UnicodeString& sb, UErrorCode&) { diff --git a/icu4c/source/i18n/number_usageprefs.cpp b/icu4c/source/i18n/number_usageprefs.cpp index 494b9687b28..0d9cb06c50a 100644 --- a/icu4c/source/i18n/number_usageprefs.cpp +++ b/icu4c/source/i18n/number_usageprefs.cpp @@ -157,7 +157,7 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m } quantity.roundToInfinity(); // Enables toDouble - const auto routed = fUnitsRouter.route(quantity.toDouble(), status); + const units::RouteResult routed = fUnitsRouter.route(quantity.toDouble(), µs.rounder, status); if (U_FAILURE(status)) { return; } @@ -168,38 +168,6 @@ void UsagePrefsHandler::processQuantity(DecimalQuantity &quantity, MicroProps &m } mixedMeasuresToMicros(routedMeasures, &quantity, µs, status); - - UnicodeString precisionSkeleton = routed.precision; - if (micros.rounder.fPrecision.isBogus()) { - if (precisionSkeleton.length() > 0) { - micros.rounder.fPrecision = parseSkeletonToPrecision(precisionSkeleton, status); - } else { - // We use the same rounding mode as COMPACT notation: known to be a - // human-friendly rounding mode: integers, but add a decimal digit - // as needed to ensure we have at least 2 significant digits. - micros.rounder.fPrecision = Precision::integer().withMinDigits(2); - } - } -} - -Precision UsagePrefsHandler::parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, - UErrorCode status) { - if (U_FAILURE(status)) { - // As a member of UsagePrefsHandler, which is a friend of Precision, we - // get access to the default constructor. - return {}; - } - constexpr int32_t kSkelPrefixLen = 20; - if (!precisionSkeleton.startsWith(UNICODE_STRING_SIMPLE("precision-increment/"))) { - status = U_INVALID_FORMAT_ERROR; - return {}; - } - U_ASSERT(precisionSkeleton[kSkelPrefixLen - 1] == u'/'); - StringSegment segment(precisionSkeleton, false); - segment.adjustOffset(kSkelPrefixLen); - MacroProps macros; - blueprint_helpers::parseIncrementOption(segment, macros, status); - return macros.precision; } UnitConversionHandler::UnitConversionHandler(const MeasureUnit &inputUnit, const MeasureUnit &outputUnit, @@ -227,19 +195,14 @@ void UnitConversionHandler::processQuantity(DecimalQuantity &quantity, MicroProp return; } quantity.roundToInfinity(); // Enables toDouble - MaybeStackVector measures = fUnitConverter->convert(quantity.toDouble(), status); + MaybeStackVector measures = + fUnitConverter->convert(quantity.toDouble(), µs.rounder, status); micros.outputUnit = fOutputUnit; if (U_FAILURE(status)) { return; } mixedMeasuresToMicros(measures, &quantity, µs, status); - - // TODO: add tests to explore behaviour that may suggest a more - // human-centric default rounder? - // if (micros.rounder.fPrecision.isBogus()) { - // micros.rounder.fPrecision = Precision::integer().withMinDigits(2); - // } } #endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/i18n/number_usageprefs.h b/icu4c/source/i18n/number_usageprefs.h index b773979339e..9e8bd936bd7 100644 --- a/icu4c/source/i18n/number_usageprefs.h +++ b/icu4c/source/i18n/number_usageprefs.h @@ -60,8 +60,6 @@ class U_I18N_API UsagePrefsHandler : public MicroPropsGenerator, public UMemory private: UnitsRouter fUnitsRouter; const MicroPropsGenerator *fParent; - - static Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, UErrorCode status); }; } // namespace impl diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index c5aeeca0d47..b0c63dc6ca7 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -99,6 +99,13 @@ class MultiplierParseHandler; } } +namespace units { + +// Forward declarations: +class UnitsRouter; + +} // namespace units + namespace number { // icu::number // Forward declarations: @@ -158,7 +165,6 @@ struct UFormattedNumberImpl; class MutablePatternModifier; class ImmutablePatternModifier; struct DecimalFormatWarehouse; -class UsagePrefsHandler; /** * Used for NumberRangeFormatter and implemented in numrange_fluent.cpp. @@ -764,7 +770,7 @@ class U_I18N_API Precision : public UMemory { friend class impl::GeneratorHelpers; // To allow access to isBogus and the default (bogus) constructor: - friend class impl::UsagePrefsHandler; + friend class units::UnitsRouter; }; /** diff --git a/icu4c/source/i18n/units_complexconverter.cpp b/icu4c/source/i18n/units_complexconverter.cpp index f145030ff35..9775b6aede7 100644 --- a/icu4c/source/i18n/units_complexconverter.cpp +++ b/icu4c/source/i18n/units_complexconverter.cpp @@ -8,6 +8,8 @@ #include #include "cmemory.h" +#include "number_decimalquantity.h" +#include "number_roundingutils.h" #include "uarrsort.h" #include "uassert.h" #include "unicode/fmtable.h" @@ -105,11 +107,24 @@ UBool ComplexUnitsConverter::greaterThanOrEqual(double quantity, double limit) c return newQuantity >= limit; } -MaybeStackVector ComplexUnitsConverter::convert(double quantity, UErrorCode &status) const { +MaybeStackVector ComplexUnitsConverter::convert(double quantity, + icu::number::impl::RoundingImpl *rounder, + UErrorCode &status) const { // TODO(icu-units#63): test negative numbers! // TODO(hugovdm): return an error for "foot-and-foot"? MaybeStackVector result; + // For N converters: + // - the first converter converts from the input unit to the largest unit, + // - N-1 converters convert to bigger units for which we want integers, + // - the Nth converter (index N-1) converts to the smallest unit, for which + // we keep a double. + MaybeStackArray intValues(unitConverters_.length() - 1, status); + if (U_FAILURE(status)) { + return result; + } + uprv_memset(intValues.getAlias(), 0, (unitConverters_.length() - 1) * sizeof(int64_t)); + for (int i = 0, n = unitConverters_.length(); i < n; ++i) { quantity = (*unitConverters_[i]).convert(quantity); if (i < n - 1) { @@ -120,11 +135,7 @@ MaybeStackVector ComplexUnitsConverter::convert(double quantity, UError // original values to ensure unbiased accuracy (to the extent of // double's capabilities). int64_t roundedQuantity = floor(quantity * (1 + DBL_EPSILON)); - Formattable formattableNewQuantity(roundedQuantity); - - // NOTE: Measure would own its MeasureUnit. - MeasureUnit *type = new MeasureUnit(units_[i]->copy(status).build(status)); - result.emplaceBackAndCheckErrorCode(status, formattableNewQuantity, type, status); + intValues[i] = roundedQuantity; // Keep the residual of the quantity. // For example: `3.6 feet`, keep only `0.6 feet` @@ -137,11 +148,76 @@ MaybeStackVector ComplexUnitsConverter::convert(double quantity, UError quantity -= roundedQuantity; } } else { // LAST ELEMENT - Formattable formattableQuantity(quantity); + if (rounder == nullptr) { + // Nothing to do for the last element. + break; + } - // NOTE: Measure would own its MeasureUnit. + // Round the last value + // TODO(ICU-21288): get smarter about precision for mixed units. + number::impl::DecimalQuantity quant; + quant.setToDouble(quantity); + rounder->apply(quant, status); + if (U_FAILURE(status)) { + return result; + } + quantity = quant.toDouble(); + if (i == 0) { + // Last element is also the first element, so we're done + break; + } + + // Check if there's a carry, and bubble it back up the resulting intValues. + int64_t carry = floor(unitConverters_[i]->convertInverse(quantity) * (1 + DBL_EPSILON)); + if (carry <= 0) { + break; + } + quantity -= unitConverters_[i]->convert(carry); + intValues[i - 1] += carry; + + // We don't use the first converter: that one is for the input unit + for (int32_t j = i - 1; j > 0; j--) { + carry = floor(unitConverters_[j]->convertInverse(intValues[j]) * (1 + DBL_EPSILON)); + if (carry <= 0) { + break; + } + intValues[j] -= round(unitConverters_[j]->convert(carry)); + intValues[j - 1] += carry; + } + } + } + + // Package values into Measure instances in result: + for (int i = 0, n = unitConverters_.length(); i < n; ++i) { + if (i < n - 1) { + Formattable formattableQuantity(intValues[i]); + // Measure takes ownership of the MeasureUnit* MeasureUnit *type = new MeasureUnit(units_[i]->copy(status).build(status)); - result.emplaceBackAndCheckErrorCode(status, formattableQuantity, type, status); + if (result.emplaceBackAndCheckErrorCode(status, formattableQuantity, type, status) == + nullptr) { + // Ownership wasn't taken + U_ASSERT(U_FAILURE(status)); + delete type; + } + if (U_FAILURE(status)) { + return result; + } + } else { // LAST ELEMENT + // Add the last element, not an integer: + Formattable formattableQuantity(quantity); + // Measure takes ownership of the MeasureUnit* + MeasureUnit *type = new MeasureUnit(units_[i]->copy(status).build(status)); + if (result.emplaceBackAndCheckErrorCode(status, formattableQuantity, type, status) == + nullptr) { + // Ownership wasn't taken + U_ASSERT(U_FAILURE(status)); + delete type; + } + if (U_FAILURE(status)) { + return result; + } + U_ASSERT(result.length() == i + 1); + U_ASSERT(result[i] != nullptr); } } @@ -153,6 +229,7 @@ MaybeStackVector ComplexUnitsConverter::convert(double quantity, UError for (int32_t i = 0; i < unitsCount; i++) { for (int32_t j = i; j < unitsCount; j++) { // Find the next expected unit, and swap it into place. + U_ASSERT(result[j] != nullptr); if (result[j]->getUnit() == *outputUnits_[i]) { if (j != i) { Measure *tmp = arr[j]; diff --git a/icu4c/source/i18n/units_complexconverter.h b/icu4c/source/i18n/units_complexconverter.h index fc355d949db..83c5b94342f 100644 --- a/icu4c/source/i18n/units_complexconverter.h +++ b/icu4c/source/i18n/units_complexconverter.h @@ -9,6 +9,7 @@ #include "cmemory.h" #include "measunit_impl.h" +#include "number_roundingutils.h" #include "unicode/errorcode.h" #include "unicode/measure.h" #include "units_converter.h" @@ -73,7 +74,8 @@ class U_I18N_API ComplexUnitsConverter : public UMemory { // NOTE: // the smallest element is the only element that could have fractional values. And all // other elements are floored to the nearest integer - MaybeStackVector convert(double quantity, UErrorCode &status) const; + MaybeStackVector + convert(double quantity, icu::number::impl::RoundingImpl *rounder, UErrorCode &status) const; private: MaybeStackVector unitConverters_; diff --git a/icu4c/source/i18n/units_converter.cpp b/icu4c/source/i18n/units_converter.cpp index 34633878872..a777d026b98 100644 --- a/icu4c/source/i18n/units_converter.cpp +++ b/icu4c/source/i18n/units_converter.cpp @@ -510,15 +510,36 @@ double UnitConverter::convert(double inputValue) const { result -= conversionRate_.targetOffset; // Set the result to its index. - if (result == 0) - return 0.0; // If the result is zero, it does not matter if the conversion are reciprocal or not. 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; + } result = 1.0 / result; } return result; } +double UnitConverter::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.) + return 0.0; + } + result = 1.0 / result; + } + result += conversionRate_.targetOffset; + result *= conversionRate_.factorDen / conversionRate_.factorNum; + result -= conversionRate_.sourceOffset; + return result; +} + } // namespace units U_NAMESPACE_END diff --git a/icu4c/source/i18n/units_converter.h b/icu4c/source/i18n/units_converter.h index 2f5dad88a0b..86764058eb2 100644 --- a/icu4c/source/i18n/units_converter.h +++ b/icu4c/source/i18n/units_converter.h @@ -144,14 +144,23 @@ class U_I18N_API UnitConverter : public UMemory { const ConversionRates &ratesInfo, UErrorCode &status); /** - * Convert a value in the source unit to another value in the target unit. + * Convert a measurement expressed in the source unit to a measurement + * expressed in the target unit. * - * @param input_value the value that needs to be converted. - * @param output_value the value that holds the result of the conversion. - * @param status + * @param inputValue the value to be converted. + * @return the converted value. */ double convert(double inputValue) const; + /** + * The inverse of convert(): convert a measurement expressed in the target + * unit to a measurement expressed in the source unit. + * + * @param inputValue the value to be converted. + * @return the converted value. + */ + double convertInverse(double inputValue) const; + private: ConversionRate conversionRate_; }; diff --git a/icu4c/source/i18n/units_router.cpp b/icu4c/source/i18n/units_router.cpp index 759ae2b744d..9489a8d8b60 100644 --- a/icu4c/source/i18n/units_router.cpp +++ b/icu4c/source/i18n/units_router.cpp @@ -10,6 +10,7 @@ #include "cstring.h" #include "measunit_impl.h" #include "number_decimalquantity.h" +#include "number_roundingutils.h" #include "resource.h" #include "unicode/measure.h" #include "units_data.h" @@ -18,6 +19,29 @@ U_NAMESPACE_BEGIN namespace units { +using number::Precision; +using number::impl::parseIncrementOption; + +Precision UnitsRouter::parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, + UErrorCode &status) { + if (U_FAILURE(status)) { + // As a member of UsagePrefsHandler, which is a friend of Precision, we + // get access to the default constructor. + return {}; + } + constexpr int32_t kSkelPrefixLen = 20; + if (!precisionSkeleton.startsWith(UNICODE_STRING_SIMPLE("precision-increment/"))) { + status = U_INVALID_FORMAT_ERROR; + return {}; + } + U_ASSERT(precisionSkeleton[kSkelPrefixLen - 1] == u'/'); + StringSegment segment(precisionSkeleton, false); + segment.adjustOffset(kSkelPrefixLen); + Precision result; + parseIncrementOption(segment, result, status); + return result; +} + UnitsRouter::UnitsRouter(MeasureUnit inputUnit, StringPiece region, StringPiece usage, UErrorCode &status) { // TODO: do we want to pass in ConversionRates and UnitPreferences instead @@ -67,22 +91,32 @@ UnitsRouter::UnitsRouter(MeasureUnit inputUnit, StringPiece region, StringPiece } } -RouteResult UnitsRouter::route(double quantity, UErrorCode &status) const { - for (int i = 0, n = converterPreferences_.length(); i < n; i++) { - const auto &converterPreference = *converterPreferences_[i]; - if (converterPreference.converter.greaterThanOrEqual(quantity * (1 + DBL_EPSILON), - converterPreference.limit)) { - return RouteResult(converterPreference.converter.convert(quantity, status), - converterPreference.precision, - converterPreference.targetUnit.copy(status)); +RouteResult UnitsRouter::route(double quantity, icu::number::impl::RoundingImpl *rounder, UErrorCode &status) const { + // Find the matching preference + const ConverterPreference *converterPreference = nullptr; + for (int32_t i = 0, n = converterPreferences_.length(); i < n; i++) { + converterPreference = converterPreferences_[i]; + if (converterPreference->converter.greaterThanOrEqual(quantity * (1 + DBL_EPSILON), + converterPreference->limit)) { + break; + } + } + U_ASSERT(converterPreference != nullptr); + + // Set up the rounder for this preference's precision + if (rounder != nullptr && rounder->fPrecision.isBogus()) { + if (converterPreference->precision.length() > 0) { + rounder->fPrecision = parseSkeletonToPrecision(converterPreference->precision, status); + } else { + // We use the same rounding mode as COMPACT notation: known to be a + // human-friendly rounding mode: integers, but add a decimal digit + // as needed to ensure we have at least 2 significant digits. + rounder->fPrecision = Precision::integer().withMinDigits(2); } } - // In case of the `quantity` does not fit in any converter limit, use the last converter. - const auto &lastConverterPreference = (*converterPreferences_[converterPreferences_.length() - 1]); - return RouteResult(lastConverterPreference.converter.convert(quantity, status), - lastConverterPreference.precision, - lastConverterPreference.targetUnit.copy(status)); + return RouteResult(converterPreference->converter.convert(quantity, rounder, status), + converterPreference->targetUnit.copy(status)); } const MaybeStackVector *UnitsRouter::getOutputUnits() const { diff --git a/icu4c/source/i18n/units_router.h b/icu4c/source/i18n/units_router.h index e6b74aa11b4..bd7a93d2d8c 100644 --- a/icu4c/source/i18n/units_router.h +++ b/icu4c/source/i18n/units_router.h @@ -21,6 +21,9 @@ U_NAMESPACE_BEGIN // Forward declarations class Measure; +namespace number { +class Precision; +} namespace units { @@ -31,19 +34,13 @@ struct RouteResult : UMemory { // TODO(icu-units/icu#21): figure out the right mixed unit API. MaybeStackVector measures; - // A skeleton string starting with a precision-increment. - // - // TODO(hugovdm): generalise? or narrow down to only a precision-increment? - // or document that other skeleton elements are ignored? - UnicodeString precision; - // The output unit for this RouteResult. This may be a MIXED unit - for // example: "yard-and-foot-and-inch", for which `measures` will have three // elements. MeasureUnitImpl outputUnit; - RouteResult(MaybeStackVector measures, UnicodeString precision, MeasureUnitImpl outputUnit) - : measures(std::move(measures)), precision(std::move(precision)), outputUnit(std::move(outputUnit)) {} + RouteResult(MaybeStackVector measures, MeasureUnitImpl outputUnit) + : measures(std::move(measures)), outputUnit(std::move(outputUnit)) {} }; /** @@ -125,7 +122,16 @@ class U_I18N_API UnitsRouter { public: UnitsRouter(MeasureUnit inputUnit, StringPiece locale, StringPiece usage, UErrorCode &status); - RouteResult route(double quantity, UErrorCode &status) const; + /** + * Performs locale and usage sensitive unit conversion. + * @param quantity The quantity to convert, expressed in terms of inputUnit. + * @param rounder If not null, this RoundingImpl will be used to do rounding + * on the converted value. If the rounder lacks an fPrecision, the + * rounder will be modified to use the preferred precision for the usage + * and locale preference, alternatively with the default precision. + * @param status Receives status. + */ + RouteResult route(double quantity, icu::number::impl::RoundingImpl *rounder, UErrorCode &status) const; /** * Returns the list of possible output units, i.e. the full set of @@ -143,6 +149,9 @@ class U_I18N_API UnitsRouter { MaybeStackVector outputUnits_; MaybeStackVector converterPreferences_; + + static number::Precision parseSkeletonToPrecision(icu::UnicodeString precisionSkeleton, + UErrorCode &status); }; } // namespace units diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index fda37de90c8..a3d059a40e1 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -1110,6 +1110,7 @@ group: unitsformatter units_data.o units_converter.o units_complexconverter.o units_router.o deps resourcebundle units_extra double_conversion number_representation formattable sort + number_rounding group: decnumber decContext.o decNumber.o diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index ecc0dcab189..ca2170d728c 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -753,21 +753,17 @@ void NumberFormatterApiTest::unitMeasure() { 4.28571, u"4 metric tons, 285 kilograms, 710 grams"); -// // TODO(icu-units#73): deal with this "1 foot 12 inches" problem. -// // At the time of writing, this test would pass, but is commented out -// // because it reflects undesired behaviour: -// assertFormatSingle( -// u"Demonstrating the \"1 foot 12 inches\" problem", -// nullptr, -// u"unit/foot-and-inch", -// NumberFormatter::with() -// .unit(MeasureUnit::forIdentifier("foot-and-inch", status)) -// .precision(Precision::maxSignificantDigits(4)) -// .unitWidth(UNUM_UNIT_WIDTH_FULL_NAME), -// Locale("en-US"), -// 1.9999, -// // This is undesireable but current behaviour: -// u"1 foot, 12 inches"); + assertFormatSingle( + u"Testing \"1 foot 12 inches\"", + nullptr, + u"unit/foot-and-inch", + NumberFormatter::with() + .unit(MeasureUnit::forIdentifier("foot-and-inch", status)) + .precision(Precision::maxSignificantDigits(4)) + .unitWidth(UNUM_UNIT_WIDTH_FULL_NAME), + Locale("en-US"), + 1.9999, + u"2 feet, 0 inches"); } void NumberFormatterApiTest::unitCompoundMeasure() { @@ -1161,6 +1157,10 @@ void NumberFormatterApiTest::unitUsage() { Locale("en-ZA"), 30500, u"350 m"); + + // 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 8053f1d5357..113269755c9 100644 --- a/icu4c/source/test/intltest/units_test.cpp +++ b/icu4c/source/test/intltest/units_test.cpp @@ -408,6 +408,10 @@ void unitsTestDataLineFn(void *context, char *fields[][2], int32_t fieldCount, U msg.clear(); msg.append("Converting 1000 ", status).append(x, status).append(" to ", status).append(y, status); unitsTest->assertEqualsNear(msg.data(), expected, got, 0.0001 * expected); + double inverted = converter.convertInverse(got); + msg.clear(); + msg.append("Converting back to ", status).append(x, status).append(" from ", status).append(y, status); + unitsTest->assertEqualsNear(msg.data(), 1000, inverted, 0.0001); } /** @@ -439,7 +443,7 @@ void UnitsTest::testConversions() { } void UnitsTest::testComplexUnitsConverter() { - IcuTestErrorCode status(*this, "UnitsTest::testComplexUnitConversions"); + IcuTestErrorCode status(*this, "UnitsTest::testComplexUnitsConverter"); ConversionRates rates(status); MeasureUnit input = MeasureUnit::getFoot(); MeasureUnit output = MeasureUnit::forIdentifier("foot-and-inch", status); @@ -449,7 +453,7 @@ void UnitsTest::testComplexUnitsConverter() { auto converter = ComplexUnitsConverter(inputImpl, outputImpl, rates, status); // Significantly less than 2.0. - MaybeStackVector measures = converter.convert(1.9999, status); + MaybeStackVector measures = converter.convert(1.9999, nullptr, status); assertEquals("measures length", 2, measures.length()); assertEquals("1.9999: measures[0] value", 1.0, measures[0]->getNumber().getDouble(status)); assertEquals("1.9999: measures[0] unit", MeasureUnit::getFoot().getIdentifier(), @@ -458,13 +462,13 @@ void UnitsTest::testComplexUnitsConverter() { assertEquals("1.9999: measures[1] unit", MeasureUnit::getInch().getIdentifier(), measures[1]->getUnit().getIdentifier()); - // TODO: consider factoring out the set of tests to make this function more + // TODO(icu-units#100): consider factoring out the set of tests to make this function more // data-driven, *after* dealing appropriately with the memory leaks that can // be demonstrated by this code. - // TODO: reusing measures results in a leak. + // TODO(icu-units#100): reusing measures results in a leak. // A minimal nudge under 2.0. - MaybeStackVector measures2 = converter.convert((2.0 - DBL_EPSILON), status); + MaybeStackVector measures2 = converter.convert((2.0 - DBL_EPSILON), nullptr, status); assertEquals("measures length", 2, measures2.length()); assertEquals("1 - eps: measures[0] value", 2.0, measures2[0]->getNumber().getDouble(status)); assertEquals("1 - eps: measures[0] unit", MeasureUnit::getFoot().getIdentifier(), @@ -480,14 +484,14 @@ void UnitsTest::testComplexUnitsConverter() { // An epsilon's nudge under one light-year: should give 1 ly, 0 m. input = MeasureUnit::getLightYear(); output = MeasureUnit::forIdentifier("light-year-and-meter", status); - // TODO: reusing tempInput and tempOutput results in a leak. + // TODO(icu-units#100): reusing tempInput and tempOutput results in a leak. MeasureUnitImpl tempInput3, tempOutput3; const MeasureUnitImpl &inputImpl3 = MeasureUnitImpl::forMeasureUnit(input, tempInput3, status); const MeasureUnitImpl &outputImpl3 = MeasureUnitImpl::forMeasureUnit(output, tempOutput3, status); - // TODO: reusing converter results in a leak. + // TODO(icu-units#100): reusing converter results in a leak. ComplexUnitsConverter converter3 = ComplexUnitsConverter(inputImpl3, outputImpl3, rates, status); - // TODO: reusing measures results in a leak. - MaybeStackVector measures3 = converter3.convert((2.0 - DBL_EPSILON), status); + // TODO(icu-units#100): reusing measures results in a leak. + MaybeStackVector measures3 = converter3.convert((2.0 - DBL_EPSILON), nullptr, status); assertEquals("measures length", 2, measures3.length()); assertEquals("light-year test: measures[0] value", 2.0, measures3[0]->getNumber().getDouble(status)); assertEquals("light-year test: measures[0] unit", MeasureUnit::getLightYear().getIdentifier(), @@ -499,7 +503,7 @@ void UnitsTest::testComplexUnitsConverter() { // 1e-15 light years is 9.46073 meters (calculated using "bc" and the CLDR // conversion factor). With double-precision maths, we get 10.5. In this // case, we're off by almost 1 meter. - MaybeStackVector measures4 = converter3.convert((1.0 + 1e-15), status); + MaybeStackVector measures4 = converter3.convert((1.0 + 1e-15), nullptr, status); assertEquals("measures length", 2, measures4.length()); assertEquals("light-year test: measures[0] value", 1.0, measures4[0]->getNumber().getDouble(status)); assertEquals("light-year test: measures[0] unit", MeasureUnit::getLightYear().getIdentifier(), @@ -511,7 +515,7 @@ void UnitsTest::testComplexUnitsConverter() { // 2e-16 light years is 1.892146 meters. We consider this in the noise, and // thus expect a 0. (This test fails when 2e-16 is increased to 4e-16.) - MaybeStackVector measures5 = converter3.convert((1.0 + 2e-16), status); + MaybeStackVector measures5 = converter3.convert((1.0 + 2e-16), nullptr, status); assertEquals("measures length", 2, measures5.length()); assertEquals("light-year test: measures[0] value", 1.0, measures5[0]->getNumber().getDouble(status)); assertEquals("light-year test: measures[0] unit", MeasureUnit::getLightYear().getIdentifier(), @@ -532,7 +536,7 @@ void UnitsTest::testComplexUnitConverterSorting() { ConversionRates conversionRates(status); ComplexUnitsConverter complexConverter(source, target, conversionRates, status); - auto measures = complexConverter.convert(10.0, status); + auto measures = complexConverter.convert(10.0, nullptr, status); U_ASSERT(measures.length() == 2); assertEquals("inch-and-foot unit 0", "inch", measures[0]->getUnit().getIdentifier()); @@ -732,7 +736,7 @@ void unitPreferencesTestDataLineFn(void *context, char *fields[][2], int32_t fie if (status.errIfFailureAndReset("Failure before router.route")) { return; } - auto routeResult = router.route(inputAmount, status); + RouteResult routeResult = router.route(inputAmount, nullptr, status); if (status.errIfFailureAndReset("router.route(inputAmount, ...)")) { return; } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/UnitConversionHandler.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/UnitConversionHandler.java index db270596c57..0612c63c41a 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/UnitConversionHandler.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/UnitConversionHandler.java @@ -8,7 +8,6 @@ import com.ibm.icu.impl.units.UnitsData; import com.ibm.icu.util.Measure; import com.ibm.icu.util.MeasureUnit; -import java.util.ArrayList; import java.util.List; /** @@ -49,7 +48,7 @@ public class UnitConversionHandler implements MicroPropsGenerator { MicroProps result = this.fParent.processQuantity(quantity); quantity.roundToInfinity(); // Enables toDouble - List measures = this.fComplexUnitConverter.convert(quantity.toBigDecimal()); + List measures = this.fComplexUnitConverter.convert(quantity.toBigDecimal(), result.rounder); result.outputUnit = this.fOutputUnit; UsagePrefsHandler.mixedMeasuresToMicros(measures, quantity, result); diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/UsagePrefsHandler.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/UsagePrefsHandler.java index 74c13b660bc..ca20045a7cd 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/UsagePrefsHandler.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/UsagePrefsHandler.java @@ -2,16 +2,13 @@ // License & terms of use: http://www.unicode.org/copyright.html package com.ibm.icu.impl.number; -import com.ibm.icu.impl.IllegalIcuArgumentException; import com.ibm.icu.impl.units.MeasureUnitImpl; import com.ibm.icu.impl.units.UnitsRouter; -import com.ibm.icu.number.Precision; import com.ibm.icu.util.Measure; import com.ibm.icu.util.MeasureUnit; import com.ibm.icu.util.ULocale; import java.math.BigDecimal; -import java.math.MathContext; import java.util.ArrayList; import java.util.List; @@ -28,46 +25,29 @@ public class UsagePrefsHandler implements MicroPropsGenerator { new UnitsRouter(MeasureUnitImpl.forIdentifier(inputUnit.getIdentifier()), locale.getCountry(), usage); } - private static Precision parseSkeletonToPrecision(String precisionSkeleton) { - final String kSuffixPrefix = "precision-increment/"; - if (!precisionSkeleton.startsWith(kSuffixPrefix)) { - throw new IllegalIcuArgumentException("precisionSkeleton is only precision-increment"); - } - - String skeleton = precisionSkeleton.substring(kSuffixPrefix.length()); - String skeletons[] = skeleton.split("/"); - BigDecimal num = new BigDecimal(skeletons[0]); - BigDecimal den = - skeletons.length == 2 ? - new BigDecimal(skeletons[1]) : - new BigDecimal("1"); - - - return Precision.increment(num.divide(den, MathContext.DECIMAL128)); - } - /** * Populates micros.mixedMeasures and modifies quantity, based on the values * in measures. */ - protected static void mixedMeasuresToMicros(List measures, DecimalQuantity quantity, MicroProps micros) { - micros.mixedMeasures = new ArrayList<>(); + protected static void + mixedMeasuresToMicros(List measures, DecimalQuantity outQuantity, MicroProps outMicros) { + outMicros.mixedMeasures = new ArrayList<>(); if (measures.size() > 1) { // For debugging - assert (micros.outputUnit.getComplexity() == MeasureUnit.Complexity.MIXED); + assert (outMicros.outputUnit.getComplexity() == MeasureUnit.Complexity.MIXED); // Check that we received the expected number of measurements: - assert measures.size() == micros.outputUnit.splitToSingleUnits().size(); + assert measures.size() == outMicros.outputUnit.splitToSingleUnits().size(); // Mixed units: except for the last value, we pass all values to the // LongNameHandler via micros->mixedMeasures. for (int i = 0, n = measures.size() - 1; i < n; i++) { - micros.mixedMeasures.add(measures.get(i)); + outMicros.mixedMeasures.add(measures.get(i)); } } // The last value (potentially the only value) gets passed on via quantity. - quantity.setToBigDecimal((BigDecimal) measures.get(measures.size()- 1).getNumber()); + outQuantity.setToBigDecimal((BigDecimal) measures.get(measures.size()- 1).getNumber()); } /** @@ -93,29 +73,12 @@ public class UsagePrefsHandler implements MicroPropsGenerator { MicroProps micros = this.fParent.processQuantity(quantity); quantity.roundToInfinity(); // Enables toDouble - final UnitsRouter.RouteResult routed = fUnitsRouter.route(quantity.toBigDecimal()); + final UnitsRouter.RouteResult routed = fUnitsRouter.route(quantity.toBigDecimal(), micros); final List routedMeasures = routed.measures; micros.outputUnit = routed.outputUnit.build(); UsagePrefsHandler.mixedMeasuresToMicros(routedMeasures, quantity, micros); - - String precisionSkeleton = routed.precision; - - assert micros.rounder != null; - - if (micros.rounder instanceof Precision.BogusRounder) { - Precision.BogusRounder rounder = (Precision.BogusRounder)micros.rounder; - if (precisionSkeleton != null && precisionSkeleton.length() > 0) { - micros.rounder = rounder.into(parseSkeletonToPrecision(precisionSkeleton)); - } else { - // We use the same rounding mode as COMPACT notation: known to be a - // human-friendly rounding mode: integers, but add a decimal digit - // as needed to ensure we have at least 2 significant digits. - micros.rounder = rounder.into(Precision.integer().withMinDigits(2)); - } - } - return micros; } } 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 aec0e5f3083..d864775cb49 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 @@ -1,11 +1,12 @@ // © 2020 and later: Unicode, Inc. and others. // License & terms of use: http://www.unicode.org/copyright.html - - package com.ibm.icu.impl.units; - +import com.ibm.icu.impl.number.DecimalQuantity; +import com.ibm.icu.impl.number.DecimalQuantity_DualStorageBCD; +import com.ibm.icu.number.Precision; import com.ibm.icu.util.Measure; +import com.ibm.icu.util.MeasureUnit; import java.math.BigDecimal; import java.math.RoundingMode; @@ -26,7 +27,10 @@ public class ComplexUnitsConverter { public static final BigDecimal EPSILON = BigDecimal.valueOf(Math.ulp(1.0)); public static final BigDecimal EPSILON_MULTIPLIER = BigDecimal.valueOf(1).add(EPSILON); private ArrayList unitConverters_; + // Individual units of mixed units, sorted big to small private ArrayList units_; + // Individual units of mixed units, sorted in desired output order + private ArrayList outputUnits_; /** * Constructor of `ComplexUnitsConverter`. @@ -40,6 +44,10 @@ public class ComplexUnitsConverter { public ComplexUnitsConverter(MeasureUnitImpl inputUnit, MeasureUnitImpl outputUnits, ConversionRates conversionRates) { units_ = outputUnits.extractIndividualUnits(); + outputUnits_ = new ArrayList<>(units_.size()); + for (MeasureUnitImpl itr : units_) { + outputUnits_.add(itr.build()); + } assert (!units_.isEmpty()); // Sort the units in a descending order. @@ -95,8 +103,16 @@ public class ComplexUnitsConverter { * the smallest element is the only element that could have fractional values. And all * other elements are floored to the nearest integer */ - public List convert(BigDecimal quantity) { - List result = new ArrayList<>(); + public List convert(BigDecimal quantity, Precision rounder) { + List result = new ArrayList<>(unitConverters_.size()); + + // For N converters: + // - the first converter converts from the input unit to the largest + // unit, + // - N-1 converters convert to bigger units for which we want integers, + // - the Nth converter (index N-1) converts to the smallest unit, which + // isn't (necessarily) an integer. + List intValues = new ArrayList<>(unitConverters_.size() - 1); for (int i = 0, n = unitConverters_.size(); i < n; ++i) { quantity = (unitConverters_.get(i)).convert(quantity); @@ -108,21 +124,85 @@ 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 newQuantity = quantity.multiply(EPSILON_MULTIPLIER).setScale(0, RoundingMode.FLOOR); - - result.add(new Measure(newQuantity, units_.get(i).build())); + BigDecimal roundedQuantity = + quantity.multiply(EPSILON_MULTIPLIER).setScale(0, RoundingMode.FLOOR); + intValues.add(roundedQuantity); // Keep the residual of the quantity. // For example: `3.6 feet`, keep only `0.6 feet` - quantity = quantity.subtract(newQuantity); + quantity = quantity.subtract(roundedQuantity); if (quantity.compareTo(BigDecimal.ZERO) == -1) { quantity = BigDecimal.ZERO; } } else { // LAST ELEMENT + if (rounder == null) { + // Nothing to do for the last element. + break; + } + + // Round the last value + // TODO(ICU-21288): get smarter about precision for mixed units. + DecimalQuantity quant = new DecimalQuantity_DualStorageBCD(quantity); + rounder.apply(quant); + quantity = quant.toBigDecimal(); + if (i == 0) { + // Last element is also the first element, so we're done + break; + } + + // Check if there's a carry, and bubble it back up the resulting intValues. + BigDecimal carry = unitConverters_.get(i) + .convertInverse(quantity) + .multiply(EPSILON_MULTIPLIER) + .setScale(0, RoundingMode.FLOOR); + if (carry.compareTo(BigDecimal.ZERO) <= 0) { // carry is not greater than zero + break; + } + quantity = quantity.subtract(unitConverters_.get(i).convert(carry)); + intValues.set(i - 1, intValues.get(i - 1).add(carry)); + + // We don't use the first converter: that one is for the input unit + for (int j = i - 1; j > 0; j--) { + carry = unitConverters_.get(j) + .convertInverse(intValues.get(j)) + .multiply(EPSILON_MULTIPLIER) + .setScale(0, RoundingMode.FLOOR); + if (carry.compareTo(BigDecimal.ZERO) <= 0) { // carry is not greater than zero + break; + } + intValues.set(j, intValues.get(j).subtract(unitConverters_.get(j).convert(carry))); + intValues.set(j - 1, intValues.get(j - 1).add(carry)); + } + } + } + + // Package values into Measure instances in result: + for (int i = 0, n = unitConverters_.size(); i < n; ++i) { + if (i < n - 1) { + result.add(new Measure(intValues.get(i), units_.get(i).build())); + } else { result.add(new Measure(quantity, units_.get(i).build())); } } + for (int i = 0; i < result.size(); i++) { + for (int j = i; j < result.size(); j++) { + // Find the next expected unit, and swap it into place. + if (result.get(j).getUnit().equals(outputUnits_.get(i))) { + if (j != i) { + Measure tmp = result.get(j); + result.set(j, result.get(i)); + result.set(i, tmp); + } + } + } + } + return result; } + + @Override + public String toString() { + return "ComplexUnitsConverter [unitConverters_=" + unitConverters_ + ", units_=" + units_ + "]"; + } } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/MeasureUnitImpl.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/MeasureUnitImpl.java index ac8b1ed187e..d46487d0cb5 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/MeasureUnitImpl.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/MeasureUnitImpl.java @@ -1,7 +1,5 @@ // © 2020 and later: Unicode, Inc. and others. // License & terms of use: http://www.unicode.org/copyright.html - - package com.ibm.icu.impl.units; import com.ibm.icu.util.BytesTrie; @@ -765,4 +763,9 @@ public class MeasureUnitImpl { return o1.compareTo(o2); } } -} \ No newline at end of file + + @Override + public String toString() { + return "MeasureUnitImpl [" + build().getIdentifier() + "]"; + } +} diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitConverter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitConverter.java index 9ac7e1c6d43..7a388523487 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitConverter.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitConverter.java @@ -1,7 +1,5 @@ // © 2020 and later: Unicode, Inc. and others. // License & terms of use: http://www.unicode.org/copyright.html - - package com.ibm.icu.impl.units; import com.ibm.icu.util.MeasureUnit; @@ -88,6 +86,10 @@ public class UnitConverter { return inputValue.multiply(this.conversionRate).add(offset); } + public BigDecimal convertInverse(BigDecimal inputValue) { + return inputValue.subtract(offset).divide(this.conversionRate, DECIMAL128); + } + public enum Convertibility { CONVERTIBLE, RECIPROCAL, @@ -308,4 +310,9 @@ public class UnitConverter { } } } + + @Override + public String toString() { + return "UnitConverter [conversionRate=" + conversionRate + ", offset=" + offset + "]"; + } } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitPreferences.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitPreferences.java index ac5d9a7d8ee..5dedeeb8f7a 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitPreferences.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitPreferences.java @@ -1,7 +1,5 @@ // © 2020 and later: Unicode, Inc. and others. // License & terms of use: http://www.unicode.org/copyright.html - - package com.ibm.icu.impl.units; import com.ibm.icu.impl.ICUData; diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsRouter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsRouter.java index 4f8fbb68636..f2ebfda6070 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsRouter.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsRouter.java @@ -1,9 +1,10 @@ // © 2020 and later: Unicode, Inc. and others. // License & terms of use: http://www.unicode.org/copyright.html - - package com.ibm.icu.impl.units; +import com.ibm.icu.impl.IllegalIcuArgumentException; +import com.ibm.icu.impl.number.MicroProps; +import com.ibm.icu.number.Precision; import com.ibm.icu.util.Measure; import com.ibm.icu.util.MeasureUnit; @@ -79,27 +80,55 @@ public class UnitsRouter { } } - public RouteResult route(BigDecimal quantity) { - for (ConverterPreference converterPreference : - converterPreferences_) { + /** If micros.rounder is a BogusRounder, this function replaces it with a valid one. */ + public RouteResult route(BigDecimal quantity, MicroProps micros) { + Precision rounder = micros == null ? null : micros.rounder; + ConverterPreference converterPreference = null; + for (ConverterPreference itr : converterPreferences_) { + converterPreference = itr; if (converterPreference.converter.greaterThanOrEqual(quantity, converterPreference.limit)) { - return new RouteResult( - converterPreference.converter.convert(quantity), - converterPreference.precision, - converterPreference.targetUnit - ); + break; + } + } + assert converterPreference != null; + assert converterPreference.precision != null; + + // Set up the rounder for this preference's precision + if (rounder != null && rounder instanceof Precision.BogusRounder) { + Precision.BogusRounder bogus = (Precision.BogusRounder)rounder; + if (converterPreference.precision.length() > 0) { + rounder = bogus.into(parseSkeletonToPrecision(converterPreference.precision)); + } else { + // We use the same rounding mode as COMPACT notation: known to be a + // human-friendly rounding mode: integers, but add a decimal digit + // as needed to ensure we have at least 2 significant digits. + rounder = bogus.into(Precision.integer().withMinDigits(2)); } } - // In case of the `quantity` does not fit in any converter limit, use the last converter. - ConverterPreference lastConverterPreference = converterPreferences_.get(converterPreferences_.size() - 1); + if (micros != null) { + micros.rounder = rounder; + } return new RouteResult( - lastConverterPreference.converter.convert(quantity), - lastConverterPreference.precision, - lastConverterPreference.targetUnit + converterPreference.converter.convert(quantity, rounder), + converterPreference.targetUnit ); } + private static Precision parseSkeletonToPrecision(String precisionSkeleton) { + final String kSkeletonPrefix = "precision-increment/"; + if (!precisionSkeleton.startsWith(kSkeletonPrefix)) { + throw new IllegalIcuArgumentException("precisionSkeleton is only precision-increment"); + } + + // TODO(icu-units#104): the C++ code uses a more sophisticated + // parseIncrementOption which supports "withMinFraction" - e.g. + // "precision-increment/0.5". Test with a unit preference that uses + // this, and fix Java. + String incrementValue = precisionSkeleton.substring(kSkeletonPrefix.length()); + return Precision.increment(new BigDecimal(incrementValue)); + } + /** * Returns the list of possible output units, i.e. the full set of * preferences, for the localized, usage-specific unit preferences. @@ -152,20 +181,13 @@ public class UnitsRouter { // TODO(icu-units/icu#21): figure out the right mixed unit API. public final List measures; - // A skeleton string starting with a precision-increment. - // - // TODO(hugovdm): generalise? or narrow down to only a precision-increment? - // or document that other skeleton elements are ignored? - public final String precision; - // The output unit for this RouteResult. This may be a MIXED unit - for // example: "yard-and-foot-and-inch", for which `measures` will have three // elements. public final MeasureUnitImpl outputUnit; - RouteResult(List measures, String precision, MeasureUnitImpl outputUnit) { + RouteResult(List measures, MeasureUnitImpl outputUnit) { this.measures = measures; - this.precision = precision; this.outputUnit = outputUnit; } } 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 3ace9271d26..69df74dc132 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 @@ -1,7 +1,5 @@ // © 2020 and later: Unicode, Inc. and others. // License & terms of use: http://www.unicode.org/copyright.html - - package com.ibm.icu.dev.test.impl; import com.ibm.icu.dev.test.TestUtil; @@ -47,7 +45,7 @@ public class UnitsTest { ComplexUnitsConverter converter = new ComplexUnitsConverter(inputImpl, outputImpl, rates); // Significantly less than 2.0. - List measures = converter.convert(BigDecimal.valueOf(1.9999)); + List measures = converter.convert(BigDecimal.valueOf(1.9999), null); assertEquals("measures length", 2, measures.size()); assertEquals("1.9999: measures[0] value", BigDecimal.valueOf(1), measures.get(0).getNumber()); assertEquals("1.9999: measures[0] unit", MeasureUnit.FOOT.getIdentifier(), @@ -58,13 +56,14 @@ public class UnitsTest { assertEquals("1.9999: measures[1] unit", MeasureUnit.INCH.getIdentifier(), measures.get(1).getUnit().getIdentifier()); - // TODO: consider factoring out the set of tests to make this function more - // data-driven, *after* dealing appropriately with the memory leaks that can - // be demonstrated by this code. + // TODO(icu-units#100): consider factoring out the set of tests to make + // this function more data-driven, *after* dealing appropriately with + // the C++ memory leaks that can be demonstrated by the C++ version of + // this code. - // TODO: reusing measures results in a leak. // A minimal nudge under 2.0. - List measures2 = converter.convert(BigDecimal.valueOf(2.0).subtract(ComplexUnitsConverter.EPSILON)); + List measures2 = + converter.convert(BigDecimal.valueOf(2.0).subtract(ComplexUnitsConverter.EPSILON), null); assertEquals("measures length", 2, measures2.size()); assertEquals("1 - eps: measures[0] value", BigDecimal.valueOf(2), measures2.get(0).getNumber()); assertEquals("1 - eps: measures[0] unit", MeasureUnit.FOOT.getIdentifier(), @@ -83,11 +82,10 @@ public class UnitsTest { final MeasureUnitImpl inputImpl3 = MeasureUnitImpl.forIdentifier(input.getIdentifier()); final MeasureUnitImpl outputImpl3 = MeasureUnitImpl.forIdentifier(output.getIdentifier()); - // TODO: reusing converter results in a leak. ComplexUnitsConverter converter3 = new ComplexUnitsConverter(inputImpl3, outputImpl3, rates); - // TODO: reusing measures results in a leak. - List measures3 = converter3.convert(BigDecimal.valueOf(2.0).subtract(ComplexUnitsConverter.EPSILON)); + List measures3 = + converter3.convert(BigDecimal.valueOf(2.0).subtract(ComplexUnitsConverter.EPSILON), null); assertEquals("measures length", 2, measures3.size()); assertEquals("light-year test: measures[0] value", BigDecimal.valueOf(2), measures3.get(0).getNumber()); assertEquals("light-year test: measures[0] unit", MeasureUnit.LIGHT_YEAR.getIdentifier(), @@ -99,7 +97,7 @@ public class UnitsTest { // 1e-15 light years is 9.46073 meters (calculated using "bc" and the CLDR // conversion factor). With double-precision maths, we get 10.5. In this // case, we're off by almost 1 meter. - List measures4 = converter3.convert(BigDecimal.valueOf(1.0 + 1e-15)); + List measures4 = converter3.convert(BigDecimal.valueOf(1.0 + 1e-15), null); assertEquals("measures length", 2, measures4.size()); assertEquals("light-year test: measures[0] value", BigDecimal.ONE, measures4.get(0).getNumber()); assertEquals("light-year test: measures[0] unit", MeasureUnit.LIGHT_YEAR.getIdentifier(), @@ -112,7 +110,7 @@ public class UnitsTest { // 2e-16 light years is 1.892146 meters. We consider this in the noise, and // thus expect a 0. (This test fails when 2e-16 is increased to 4e-16.) - List measures5 = converter3.convert(BigDecimal.valueOf(1.0 + 2e-17)); + List measures5 = converter3.convert(BigDecimal.valueOf(1.0 + 2e-17), null); assertEquals("measures length", 2, measures5.size()); assertEquals("light-year test: measures[0] value", BigDecimal.ONE, measures5.get(0).getNumber()); assertEquals("light-year test: measures[0] unit", MeasureUnit.LIGHT_YEAR.getIdentifier(), @@ -134,14 +132,14 @@ public class UnitsTest { ConversionRates conversionRates = new ConversionRates(); ComplexUnitsConverter complexConverter = new ComplexUnitsConverter(source, target, conversionRates); - List measures = complexConverter.convert(BigDecimal.valueOf(10.0)); + List measures = complexConverter.convert(BigDecimal.valueOf(10.0), null); assertEquals(measures.size(), 2); - assertEquals("inch-and-foot unit 0", "foot", measures.get(0).getUnit().getIdentifier()); - assertEquals("inch-and-foot unit 1", "inch", measures.get(1).getUnit().getIdentifier()); + assertEquals("inch-and-foot unit 0", "inch", measures.get(0).getUnit().getIdentifier()); + assertEquals("inch-and-foot unit 1", "foot", measures.get(1).getUnit().getIdentifier()); - assertTrue("inch-and-foot value 0", compareTwoBigDecimal(BigDecimal.valueOf(32), BigDecimal.valueOf(measures.get(0).getNumber().doubleValue()), BigDecimal.valueOf(0.0001))); - assertTrue("inch-and-foot value 1", compareTwoBigDecimal(BigDecimal.valueOf(9.7008), BigDecimal.valueOf(measures.get(1).getNumber().doubleValue()), BigDecimal.valueOf(0.0001))); + assertEquals("inch-and-foot value 0", 9.7008, measures.get(0).getNumber().doubleValue(), 0.0001); + assertEquals("inch-and-foot value 1", 32, measures.get(1).getNumber().doubleValue(), 0.0001); } @@ -254,21 +252,36 @@ public class UnitsTest { for (TestCase testCase : tests) { UnitConverter converter = new UnitConverter(testCase.source, testCase.target, conversionRates); - if (compareTwoBigDecimal(testCase.expected, converter.convert(testCase.input), BigDecimal.valueOf(0.000001))) { + BigDecimal got = converter.convert(testCase.input); + if (compareTwoBigDecimal(testCase.expected, got, BigDecimal.valueOf(0.000001))) { continue; } else { fail(new StringBuilder() .append(testCase.category) - .append(" ") + .append(": Converting 1000 ") .append(testCase.sourceString) - .append(" ") + .append(" to ") .append(testCase.targetString) - .append(" ") - .append(converter.convert(testCase.input).toString()) - .append(" expected ") + .append(", got ") + .append(got) + .append(", expected ") .append(testCase.expected.toString()) .toString()); } + BigDecimal inverted = converter.convertInverse(testCase.input); + if (compareTwoBigDecimal(BigDecimal.valueOf(1000), inverted, BigDecimal.valueOf(0.000001))) { + continue; + } else { + fail(new StringBuilder() + .append("Converting back to ") + .append(testCase.sourceString) + .append(" from ") + .append(testCase.targetString) + .append(": got ") + .append(inverted) + .append(", expected 1000") + .toString()); + } } } @@ -345,7 +358,7 @@ public class UnitsTest { for (TestCase testCase : tests) { UnitsRouter router = new UnitsRouter(testCase.inputUnit.second, testCase.region, testCase.usage); - List measures = router.route(testCase.input).measures; + List measures = router.route(testCase.input, null).measures; assertEquals("Measures size must be the same as expected units", measures.size(), testCase.expectedInOrder.size()); 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 3bed7ba80de..22bd114359c 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 @@ -715,21 +715,17 @@ public class NumberFormatterApiTest extends TestFmwk { 4.28571, "4 metric tons, 285 kilograms, 710 grams"); -// // TODO(icu-units#73): deal with this "1 foot 12 inches" problem. -// // At the time of writing, this test would pass, but is commented out -// // because it reflects undesired behaviour: -// assertFormatSingle( -// u"Demonstrating the \"1 foot 12 inches\" problem", -// nullptr, -// u"unit/foot-and-inch", -// NumberFormatter::with() -// .unit(MeasureUnit::forIdentifier("foot-and-inch")) -// .precision(Precision::maxSignificantDigits(4)) -// .unitWidth(UNUM_UNIT_WIDTH_FULL_NAME), -// Locale("en-US"), -// 1.9999, -// // This is undesireable but current behaviour: -// u"1 foot, 12 inches"); + assertFormatSingle( + "Testing \"1 foot 12 inches\"", + null, + "unit/foot-and-inch", + NumberFormatter.with() + .unit(MeasureUnit.forIdentifier("foot-and-inch")) + .precision(Precision.maxSignificantDigits(4)) + .unitWidth(UnitWidth.FULL_NAME), + new ULocale("en-US"), + 1.9999, + "2 feet, 0 inches"); } @Test @@ -1118,7 +1114,11 @@ public class NumberFormatterApiTest extends TestFmwk { new ULocale("en-ZA"), 30500, "350 m"); -} + + // 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