ICU-21451 Implement inverse unit handling for new consumption unit preferences in CLDR
See #1550
diff --git a/icu4c/source/data/misc/units.txt b/icu4c/source/data/misc/units.txt
index 9941a20..f8c8645 100644
--- a/icu4c/source/data/misc/units.txt
+++ b/icu4c/source/data/misc/units.txt
@@ -925,6 +925,16 @@
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 @@
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 @@
unit{"meter"}
}
{
+ geq{"10"}
skeleton{"precision-increment/10"}
unit{"meter"}
}
{
+ skeleton{"precision-increment/1"}
unit{"meter"}
}
}
@@ -1346,6 +1332,12 @@
unit{"yard"}
}
{
+ geq{"10"}
+ skeleton{"precision-increment/10"}
+ unit{"yard"}
+ }
+ {
+ skeleton{"precision-increment/1"}
unit{"yard"}
}
}
@@ -1362,9 +1354,14 @@
unit{"meter"}
}
{
+ geq{"10"}
skeleton{"precision-increment/10"}
unit{"meter"}
}
+ {
+ skeleton{"precision-increment/1"}
+ unit{"meter"}
+ }
}
US{
{
@@ -1377,9 +1374,14 @@
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 42bd624..2d94a85 100644
--- a/icu4c/source/i18n/units_data.cpp
+++ b/icu4c/source/i18n/units_data.cpp
@@ -282,6 +282,10 @@
if (U_FAILURE(status)) { return -1; }
if (idx >= 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 @@
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 @@
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 3158718..882077b 100644
--- a/icu4c/source/i18n/units_router.cpp
+++ b/icu4c/source/i18n/units_router.cpp
@@ -56,7 +56,7 @@
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 b3bfb87..e06ab57 100644
--- a/icu4c/source/test/intltest/numbertest_api.cpp
+++ b/icu4c/source/test/intltest/numbertest_api.cpp
@@ -1239,7 +1239,7 @@
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 @@
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 1846451..405c4e4 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 @@
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 3d2f933..91c62cb 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; 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 92496e4..4ef5479 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 @@
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 7391f56..19ef8f7 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 @@
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 74ccb00..4d9cc23 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 30b714f..08986d6 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 e703923..1315090 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 e9abbc2..f0899a1 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 void testUnitPreferencesWithCLDRTests() throws IOException {
class TestCase {
+ // TODO: content of outputUnitInOrder isn't checked? Only size?
final ArrayList<Pair<String, MeasureUnitImpl>> outputUnitInOrder = new ArrayList<>();
final ArrayList<BigDecimal> expectedInOrder = new ArrayList<>();
/**
@@ -486,6 +487,11 @@
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 @@
.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 0912f15..35b62e9 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 @@
"8,8 km",
"900 m",
"90 m",
- "10 m",
+ "9 m",
"0 m");
uTestCase = "unitUsage() en-GB road";
@@ -1250,8 +1250,8 @@
"54 mi",
"5.4 mi",
"0.54 mi",
- "96 yd",
- "9.6 yd",
+ "100 yd",
+ "10 yd",
"0 yd");
uTestCase = "unitUsage() en-US road";
