From 8e695522287eb17b4e0f381a3e5a4720ffd8b342 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 2 Feb 2021 15:09:25 +0000 Subject: [PATCH] ICU-21451 Implement inverse unit handling for new consumption unit preferences in CLDR See #1550 --- icu4c/source/data/misc/units.txt | 54 ++++++++++--------- icu4c/source/i18n/units_data.cpp | 18 +++++-- icu4c/source/i18n/units_router.cpp | 2 +- icu4c/source/test/intltest/numbertest_api.cpp | 6 +-- .../source/test/intltest/units_data_test.cpp | 13 +++-- .../cldr/units/unitPreferencesTest.txt | 20 ++++--- .../ibm/icu/impl/units/UnitPreferences.java | 4 +- .../src/com/ibm/icu/impl/units/UnitsData.java | 9 ++-- icu4j/main/shared/data/icudata.jar | 4 +- icu4j/main/shared/data/icutzdata.jar | 4 +- icu4j/main/shared/data/testdata.jar | 4 +- .../com/ibm/icu/dev/test/impl/UnitsTest.java | 8 ++- .../test/number/NumberFormatterApiTest.java | 6 +-- 13 files changed, 90 insertions(+), 62 deletions(-) diff --git a/icu4c/source/data/misc/units.txt b/icu4c/source/data/misc/units.txt index 9941a20949b..f8c86457d29 100644 --- a/icu4c/source/data/misc/units.txt +++ b/icu4c/source/data/misc/units.txt @@ -925,6 +925,16 @@ units:table(nofallback){ unit{"liter-per-kilometer"} } } + CA{ + { + unit{"mile-per-gallon-imperial"} + } + } + GB{ + { + unit{"mile-per-gallon-imperial"} + } + } IT{ { unit{"liter-per-kilometer"} @@ -965,32 +975,6 @@ units:table(nofallback){ unit{"liter-per-kilometer"} } } - } - } - "consumption-inverse"{ - "default"{ - 001{ - { - unit{"kilometer-per-centiliter"} - } - } - } - "vehicle-fuel"{ - 001{ - { - unit{"kilometer-per-centiliter"} - } - } - CA{ - { - unit{"mile-per-gallon-imperial"} - } - } - GB{ - { - unit{"mile-per-gallon-imperial"} - } - } US{ { unit{"mile-per-gallon"} @@ -1328,10 +1312,12 @@ units:table(nofallback){ unit{"meter"} } { + geq{"10"} skeleton{"precision-increment/10"} unit{"meter"} } { + skeleton{"precision-increment/1"} unit{"meter"} } } @@ -1346,6 +1332,12 @@ units:table(nofallback){ unit{"yard"} } { + geq{"10"} + skeleton{"precision-increment/10"} + unit{"yard"} + } + { + skeleton{"precision-increment/1"} unit{"yard"} } } @@ -1362,9 +1354,14 @@ units:table(nofallback){ unit{"meter"} } { + geq{"10"} skeleton{"precision-increment/10"} unit{"meter"} } + { + skeleton{"precision-increment/1"} + unit{"meter"} + } } US{ { @@ -1377,9 +1374,14 @@ units:table(nofallback){ unit{"foot"} } { + geq{"10"} skeleton{"precision-increment/10"} unit{"foot"} } + { + skeleton{"precision-increment/1"} + unit{"foot"} + } } } "snowfall"{ diff --git a/icu4c/source/i18n/units_data.cpp b/icu4c/source/i18n/units_data.cpp index 42bd6248b0b..2d94a851a18 100644 --- a/icu4c/source/i18n/units_data.cpp +++ b/icu4c/source/i18n/units_data.cpp @@ -282,6 +282,10 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector= 0) { return idx; } if (!foundCategory) { + // TODO: failures can happen if units::getUnitCategory returns a category + // that does not appear in unitPreferenceData. Do we want a unit test that + // checks unitPreferenceData has full coverage of categories? Or just trust + // CLDR? status = U_ILLEGAL_ARGUMENT_ERROR; return -1; } @@ -370,12 +374,12 @@ CharString U_I18N_API getUnitCategory(const char *baseUnitIdentifier, UErrorCode const UChar *uCategory = ures_getStringByKey(unitQuantities.getAlias(), baseUnitIdentifier, &categoryLength, &status); if (U_FAILURE(status)) { - // TODO(CLDR-13787,hugovdm): special-casing the consumption-inverse - // case. Once CLDR-13787 is clarified, this should be generalised (or - // possibly removed): + // TODO(icu-units#130): support inverting any unit, with correct + // fallback logic: inversion and fallback may depend on presence or + // absence of a usage for that category. if (uprv_strcmp(baseUnitIdentifier, "meter-per-cubic-meter") == 0) { status = U_ZERO_ERROR; - result.append("consumption-inverse", status); + result.append("consumption", status); return result; } } @@ -415,7 +419,11 @@ void U_I18N_API UnitPreferences::getPreferencesFor(StringPiece category, StringP const UnitPreference *const *&outPreferences, int32_t &preferenceCount, UErrorCode &status) const { int32_t idx = getPreferenceMetadataIndex(&metadata_, category, usage, region, status); - if (U_FAILURE(status)) { return; } + if (U_FAILURE(status)) { + outPreferences = nullptr; + preferenceCount = 0; + return; + } U_ASSERT(idx >= 0); // Failures should have been taken care of by `status`. const UnitPreferenceMetadata *m = metadata_[idx]; outPreferences = unitPrefs_.getAlias() + m->prefsOffset; diff --git a/icu4c/source/i18n/units_router.cpp b/icu4c/source/i18n/units_router.cpp index 3158718fd22..882077bc2dd 100644 --- a/icu4c/source/i18n/units_router.cpp +++ b/icu4c/source/i18n/units_router.cpp @@ -56,7 +56,7 @@ UnitsRouter::UnitsRouter(MeasureUnit inputUnit, StringPiece region, StringPiece CharString category = getUnitCategory(baseUnit.getIdentifier(), status); const UnitPreference *const *unitPreferences; - int32_t preferencesCount; + int32_t preferencesCount = 0; prefs.getPreferencesFor(category.data(), usage, region, unitPreferences, preferencesCount, status); for (int i = 0; i < preferencesCount; ++i) { diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index b3bfb87a9d1..e06ab577350 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -1239,7 +1239,7 @@ void NumberFormatterApiTest::unitUsage() { u"8,8 km", u"900 m", u"90 m", - u"10 m", + u"9 m", u"0 m"); uTestCase = u"unitUsage() en-GB road"; @@ -1274,8 +1274,8 @@ void NumberFormatterApiTest::unitUsage() { u"54 mi", u"5.4 mi", u"0.54 mi", - u"96 yd", - u"9.6 yd", + u"100 yd", + u"10 yd", u"0 yd"); uTestCase = u"unitUsage() en-US road"; diff --git a/icu4c/source/test/intltest/units_data_test.cpp b/icu4c/source/test/intltest/units_data_test.cpp index 18464516146..405c4e4a5b8 100644 --- a/icu4c/source/test/intltest/units_data_test.cpp +++ b/icu4c/source/test/intltest/units_data_test.cpp @@ -10,6 +10,7 @@ using namespace ::icu::units; +// These test are no in ICU4J. TODO: consider porting them to Java? class UnitsDataTest : public IntlTest { public: UnitsDataTest() {} @@ -38,12 +39,14 @@ void UnitsDataTest::testGetUnitCategory() { const char *expectedCategory; } testCases[]{ {"kilogram-per-cubic-meter", "mass-density"}, + {"cubic-meter-per-kilogram", "specific-volume"}, + {"meter-per-second", "speed"}, + // TODO(icu-units#130): inverse-speed + // {"second-per-meter", "speed"}, + // Consumption specifically supports inverse units (mile-per-galon, + // liter-per-100-kilometer): {"cubic-meter-per-meter", "consumption"}, - // TODO(CLDR-13787,hugovdm): currently we're treating - // consumption-inverse as a separate category. Once consumption - // preference handling has been clarified by CLDR-13787, this function - // should be fixed. - {"meter-per-cubic-meter", "consumption-inverse"}, + {"meter-per-cubic-meter", "consumption"}, }; IcuTestErrorCode status(*this, "testGetUnitCategory"); diff --git a/icu4c/source/test/testdata/cldr/units/unitPreferencesTest.txt b/icu4c/source/test/testdata/cldr/units/unitPreferencesTest.txt index 3d2f9336264..91c62cb7c90 100644 --- a/icu4c/source/test/testdata/cldr/units/unitPreferencesTest.txt +++ b/icu4c/source/test/testdata/cldr/units/unitPreferencesTest.txt @@ -83,14 +83,22 @@ consumption; vehicle-fuel; BR; 11 / 10000000; 1.1E-6; cubic-meter-per-meter; 11 consumption; vehicle-fuel; BR; 1 / 1000000; 1.0E-6; cubic-meter-per-meter; 1; 1.0; liter-per-kilometer consumption; vehicle-fuel; BR; 9 / 10000000; 9.0E-7; cubic-meter-per-meter; 9 / 10; 0.9; liter-per-kilometer -consumption-inverse; default; 001; 110000000; 1.1E8; meter-per-cubic-meter; 11 / 10; 1.1; kilometer-per-centiliter -consumption-inverse; default; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; kilometer-per-centiliter -consumption-inverse; default; 001; 90000000; 9.0E7; meter-per-cubic-meter; 9 / 10; 0.9; kilometer-per-centiliter +# TODO: these are local ICU-specific edits until the CLDR test file is updated: +# consumption-inverse; default; 001; 110000000; 1.1E8; meter-per-cubic-meter; 11 / 10; 1.1; kilometer-per-centiliter +consumption; default; 001; 110000000; 1.1E8; meter-per-cubic-meter; 10 / 11; 0.90909090909090906; liter-per-100-kilometer +# consumption-inverse; default; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; kilometer-per-centiliter +consumption; default; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; liter-per-100-kilometer +# consumption-inverse; default; 001; 90000000; 9.0E7; meter-per-cubic-meter; 9 / 10; 0.9; kilometer-per-centiliter +consumption; default; 001; 90000000; 9.0E7; meter-per-cubic-meter; 10 / 9; 1.1111111111111112; liter-per-100-kilometer -consumption-inverse; vehicle-fuel; 001; 110000000; 1.1E8; meter-per-cubic-meter; 11 / 10; 1.1; kilometer-per-centiliter -consumption-inverse; vehicle-fuel; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; kilometer-per-centiliter -consumption-inverse; vehicle-fuel; 001; 90000000; 9.0E7; meter-per-cubic-meter; 9 / 10; 0.9; kilometer-per-centiliter +# consumption-inverse; vehicle-fuel; 001; 110000000; 1.1E8; meter-per-cubic-meter; 11 / 10; 1.1; kilometer-per-centiliter +consumption; vehicle-fuel; 001; 110000000; 1.1E8; meter-per-cubic-meter; 10 / 11; 0.90909090909090906; liter-per-100-kilometer +# consumption-inverse; vehicle-fuel; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; kilometer-per-centiliter +consumption; vehicle-fuel; 001; 100000000; 1.0E8; meter-per-cubic-meter; 1; 1.0; liter-per-100-kilometer +# consumption-inverse; vehicle-fuel; 001; 90000000; 9.0E7; meter-per-cubic-meter; 9 / 10; 0.9; kilometer-per-centiliter +consumption; vehicle-fuel; 001; 90000000; 9.0E7; meter-per-cubic-meter; 10 / 9; 1.1111111111111112; liter-per-100-kilometer +# TODO: these still pass, as expected/desired. Leaving them categorised as consumption-inverse, this is ignored by tests anyway consumption-inverse; vehicle-fuel; US; 52800000000 / 112903; 467658.0781732992; meter-per-cubic-meter; 11 / 10; 1.1; mile-per-gallon consumption-inverse; vehicle-fuel; US; 48000000000 / 112903; 425143.707430272; meter-per-cubic-meter; 1; 1.0; mile-per-gallon consumption-inverse; vehicle-fuel; US; 43200000000 / 112903; 382629.3366872448; meter-per-cubic-meter; 9 / 10; 0.9; mile-per-gallon 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 92496e4210e..4ef5479961e 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 @@ -64,7 +64,9 @@ public class UnitPreferences { result = getUnitPreferences(category, subUsage, region); if (result != null) break; } - + // TODO: if a category is missing, we get an assertion failure, or we + // return null, causing a NullPointerException. In C++, we return an + // U_MISSING_RESOURCE_ERROR error. assert (result != null) : "At least the category must be exist"; return result; } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsData.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsData.java index 7391f56749b..19ef8f70702 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsData.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsData.java @@ -64,11 +64,10 @@ public class UnitsData { String baseUnitIdentifier = MeasureUnit.fromMeasureUnitImpl(baseMeasureUnit).getIdentifier(); if (baseUnitIdentifier.equals("meter-per-cubic-meter")) { - // TODO(CLDR-13787,hugovdm): special-casing the consumption-inverse - // case. Once CLDR-13787 is clarified, this should be generalised (or - // possibly removed): - - return "consumption-inverse"; + // TODO(icu-units#130): support inverting any unit, with correct + // fallback logic: inversion and fallback may depend on presence or + // absence of a usage for that category. + return "consumption"; } return this.categories.mapFromUnitToCategory.get(baseUnitIdentifier); diff --git a/icu4j/main/shared/data/icudata.jar b/icu4j/main/shared/data/icudata.jar index 74ccb00aa41..4d9cc237296 100644 --- a/icu4j/main/shared/data/icudata.jar +++ b/icu4j/main/shared/data/icudata.jar @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:a4adc39da0522d36906b035fe47aa7a9becfefaa4a5ccef2cdf6f5c23d194777 -size 13306768 +oid sha256:e738e530bcd2dcafff1de1d603c79d5a1edc04c095ca52366259c354f19e56ed +size 13306751 diff --git a/icu4j/main/shared/data/icutzdata.jar b/icu4j/main/shared/data/icutzdata.jar index 30b714fd6ba..08986d63d9e 100644 --- a/icu4j/main/shared/data/icutzdata.jar +++ b/icu4j/main/shared/data/icutzdata.jar @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:4dce4d778b403e5b91fbe11eba9e0b342c9fe28f2c8d39c7ae8641b311c9a71f -size 95083 +oid sha256:19f02ee2a2dc722a729fa9258175a738fc6021d252769b85c023a927135c7c26 +size 95080 diff --git a/icu4j/main/shared/data/testdata.jar b/icu4j/main/shared/data/testdata.jar index e7039236d82..1315090b2fb 100644 --- a/icu4j/main/shared/data/testdata.jar +++ b/icu4j/main/shared/data/testdata.jar @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:c80f58afc2714a7e1af4706c075a23ae37de228f0d383bf0cbaec340ade76030 -size 726478 +oid sha256:1970fbcc18ec8a8b86702fe73ffbba842e9379bd973edbfb4e189ac6ac6d2a83 +size 723496 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 e9abbc2c173..f0899a1359e 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 @@ -432,6 +432,7 @@ public class UnitsTest { public void testUnitPreferencesWithCLDRTests() throws IOException { class TestCase { + // TODO: content of outputUnitInOrder isn't checked? Only size? final ArrayList> outputUnitInOrder = new ArrayList<>(); final ArrayList expectedInOrder = new ArrayList<>(); /** @@ -486,6 +487,11 @@ public class UnitsTest { expectedInOrder.add(new BigDecimal(output.second)); } } + + public String toString() { + return "TestCase: " + category + ", " + usage + ", " + region + + "; Input: " + input + " " + inputUnit.first + "; Expected Values: " + expectedInOrder; + } } // Read Test data from the unitPreferencesTest @@ -517,7 +523,7 @@ public class UnitsTest { .compareTwoBigDecimal(testCase.expectedInOrder.get(i), BigDecimal.valueOf(measures.get(i).getNumber().doubleValue()), BigDecimal.valueOf(0.00001))) { - fail(testCase.toString() + measures.toString()); + fail("Test failed: " + testCase + "; Got unexpected result: " + measures); } } } 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 0912f150ecf..35b62e93c11 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 @@ -1218,7 +1218,7 @@ public class NumberFormatterApiTest extends TestFmwk { "8,8 km", "900 m", "90 m", - "10 m", + "9 m", "0 m"); uTestCase = "unitUsage() en-GB road"; @@ -1250,8 +1250,8 @@ public class NumberFormatterApiTest extends TestFmwk { "54 mi", "5.4 mi", "0.54 mi", - "96 yd", - "9.6 yd", + "100 yd", + "10 yd", "0 yd"); uTestCase = "unitUsage() en-US road";