Inflection-188 Support PronounConcept in icu::message2 Inflection-187 Support SemanticConcept in icu::message2 for custom vocabulary Inflection-186 Support the print and speak lines from SpeakableString in icu::message2 Inflection-185 Support lists in icu::message2 Inflection-183 Support quantify in icu::message2 Inflection-178 Provide quantity support for Serbian Inflection-117 Support inflection functionality into icu::message2 Inflection-87 Integrate Unicode Inflection into Unicode Message Format Support more languages#194
Conversation
|
|
||
| // Croatian (hr) and Serbian (sr) share Bosnian-Croatian-Serbian numeral-noun agreement: the 2-4 | ||
| // paucal takes the genitive singular and 5+ takes the genitive plural. | ||
| SerboCroatianCommonConceptFactory::SerboCroatianCommonConceptFactory(const ::inflection::util::ULocale& language) |
There was a problem hiding this comment.
Is there something I need to change or is Serbian going to pick it up by default (I can look at Croatian for guidance).
There was a problem hiding this comment.
I don't think that there is anything left to change in this class. Croatian is using the same class now. Both are registered in LocalizedCommonConceptFactoryProvider.cpp.
| @@ -0,0 +1,195 @@ | |||
| <?xml version="1.0" encoding="UTF-8" standalone="no"?> | |||
There was a problem hiding this comment.
The data driven tests in this directory has a lot of examples of how to integrate Unicode Inflection into MF2.
| <params> | ||
| <param name="object" type="String"><value>lights</value></param> | ||
| </params> | ||
| <source>The {$object} {is :inflect number=$object} on.</source> | ||
| <print>The lights are on.</print> |
There was a problem hiding this comment.
This is how you inflect a word based on another word.
| <params> | ||
| <param name="object" type="String"><value>lights</value></param> | ||
| </params> | ||
| <source>{$object :inflect definiteness=definite} {is :inflect number=$object} on.</source> | ||
| <print>the lights are on.</print> | ||
| </test> |
There was a problem hiding this comment.
Add "the" in front of a word. This is boring in English. It's more interesting in the rest of the European languages.
| <params> | ||
| <param name="object" type="String"><value>light</value></param> | ||
| </params> | ||
| <source>.local $num = {$object :inflect to=number} .match $num | ||
| plural {{The {$object} are on}} | ||
| * {{The {$object} is on}} | ||
| </source> | ||
| <print>The light is on</print> | ||
| </test> |
There was a problem hiding this comment.
This is the verbose way to implement this functionality. I'm not a fan of it, but it shows that the property can be queried for a more complex phrase.
| const inflection::dialog::SemanticFeatureModel* currModel = nullptr; | ||
| std::atomic finished(false); | ||
| std::barrier barrier(processorCount + 1); | ||
| for (int32_t count = 0; count < processorCount; count++) { | ||
| results[count].reserve(minimumNumberOfWords); | ||
| threads.emplace_back(testInflection, &currModel, &phrases, &constraints, &semanticFeatures, &barrier, &finished, count, &results[count]); | ||
| } |
There was a problem hiding this comment.
We're reusing the threads and modernizing the style of thread safety checking.
| * @brief Provides utility functions for use arrays. | ||
| * @details Provides utility functions for use arrays. | ||
| */ | ||
| class inflection::util::ArrayUtils { |
There was a problem hiding this comment.
This helps reduce the heap usage a fair amount. This allows you to perform a binary search in an arbitrary sorted constexpr C data array, and it's verified as sorted at compile time. You get a compiler error if it's unsorted.
| @@ -4,104 +4,112 @@ | |||
| #pragma once | |||
|
|
|||
| #include <inflection/grammar/synthesis/fwd.hpp> | |||
| #include <string> | |||
|
|
|||
| class inflection::grammar::synthesis::GrammemeConstants final | |||
There was a problem hiding this comment.
This is now a header only class. It avoids permanently caching these strings on the heap, and there is no thread safe initialization to be concerned about.
| * icu::messsage2::MFFunctionRegistry::Builder::adoptFormatter | ||
| * to register a custom formatter factory. | ||
| */ | ||
| static icu::message2::FormatterFactory* createInflectFormatterFactory(OutputMode mode = OutputMode::PLAIN); |
There was a problem hiding this comment.
By default, we format as plain text. If you want to retain the spoken form, then it can be outputted as SSML. This is an API breaking change.
As a reminder, this is a pre-1.0 release. So API stability guarantees are not there yet.
| OUTPUT_FILE /dev/stdout | ||
| ERROR_FILE /dev/stderr | ||
|
|
||
| if(WIN32) |
There was a problem hiding this comment.
If someone could run this on Windows, that would be helpful. The default is to print in color, but on Windows, we don't run the sh shell. I don't know if this test runs in color. This can be verified by manually running this from the command line outside of the makefiles.
| if (toFormat.getType() == UFMT_DOUBLE) { | ||
| num = std::make_unique<inflection::dialog::NumberConcept>( | ||
| toFormat.getDouble(errorCode), locale, locale); | ||
| } | ||
| else { | ||
| num = std::make_unique<inflection::dialog::NumberConcept>( | ||
| toFormat.getInt64(errorCode), locale, locale); | ||
| } |
There was a problem hiding this comment.
We recently had a meeting in ICU around this topic. There are times that formatting a number and speaking a number need to be a bit different in such scenarios. This should not be doubling up the locale usage like this. When formatting context allows this value to be decoupled, we should modify these lines.
This is a temporary workaround for now.
| <!-- FIXME: <test><source number="singular" case="illative">Lahti</source><result>Lahteen</result></test> --> | ||
| <!-- FIXME: <test><source number="singular" case="adessive">Lahti</source><result>Lahdella</result></test> --> | ||
| <!-- FIXME: <test><source number="singular" case="ablative">Lahti</source><result>Lahdelta</result></test> --> | ||
| <!-- FIXME: <test><source number="singular" case="allative">Lahti</source><result>Lahdelle</result></test> --> |
There was a problem hiding this comment.
Due to the lack of Wikidata lexeme coverage, there are numerous tests that are failing, especially for the new languages. These tests have a FIXME tag on each failing test. There are over a thousand of these data coverage issues, and the vast majority of them are in Finnish.
For all languages, adding/fixing the missing data is usually the best way to fix the issue.
For Finnish, Polish, and Russian, adding the missing words mentioned in the suffix tables will allow numerous words to be supported en masse. Addressing that data coverage issue would allow the Exemplar test to be enabled for that language.
Occasionally, the issue is not a data coverage issue, and it's an algorithm or data structure issue. There are a handful of these issues. The Norwegian and Dutch failures may fall into this category, but I'm not sure yet.
nciric
left a comment
There was a problem hiding this comment.
qq about "The existing lexical dictionaries are updated."
I assume no lexemes were added to the dictionaries that don't already exist in Wikidata. If that's not the case, we'll have a problem next time we sync with Wikidata as it will override items.
I don't quite understand the question. I did not manually edit the data derived from Wikidata, if that is what you mean. The data is derived like this, and this is how you update the lexical dictionaries. Note: I have to update this data once more before the final review. |
| return {inflectedWord}; | ||
| static constexpr const char16_t* const doNotInflect[] = { | ||
| // This must be in sorted order. | ||
| u"августа", |
There was a problem hiding this comment.
Why are month names hardcoded?
There was a problem hiding this comment.
That's a good question. I think this may be a misnomer from a previous version of this code.
It should probably be renamed as isNotGuessable or something like that because it's only used in guessInflection. Though if the words are complete, then it's just dead code/data. This needs to be revisited before merging.
There was a problem hiding this comment.
The overall change looks good - once this is fixed (and maybe Romanian dictionary) feel free to land.
There was a problem hiding this comment.
After further review, I think this is fine as is. It looks like if you try to change these genitive forms to accusative for dates, they stay unchanged according to one of the tests. Since this part is refactor, and it was already here, I recommend leaving this as-is. I'm fine with revisiting this separately with a Russian expert.
|
|
||
| bool Inflector_InflectionPattern::containsSuffix(std::u16string_view suffix) const | ||
| { | ||
| const auto suffixID = inflectorDictionary.inflectionSuffixes.getIdentifierIfAvailable(suffix); |
There was a problem hiding this comment.
This is where we "guess" word inflection if not in dictionary by suffix in inflectional_xx.xml?
There was a problem hiding this comment.
No, check Polish, Finnish, and Russian. They use a suffix file of exemplar words. It's mostly automatically generated. Some manual edits are performed on the derived suffix files. Unfortunately, the suffix files are from a more complete lexical dictionary. They should probably be rederived from Wikidata, but that should wait till after the data in Wikidata has better coverage. The ExemplarTest#testExemplarsInDictionary test has a TODO to work on this task.
Right now, it has partial word coverage, which is why the ExemplarTest#testExemplarsInDictionary test fails.
|
|
||
| // Croatian (hr) and Serbian (sr) share Bosnian-Croatian-Serbian numeral-noun agreement: the 2-4 | ||
| // paucal takes the genitive singular and 5+ takes the genitive plural. | ||
| SerboCroatianCommonConceptFactory::SerboCroatianCommonConceptFactory(const ::inflection::util::ULocale& language) |
There was a problem hiding this comment.
Is there something I need to change or is Serbian going to pick it up by default (I can look at Croatian for guidance).
I understand the process, my questions was about data source:
|
It's from Wikidata with some filtering and augmentation from ParseWikidata (dictionary-parser). That tool does augment some data that isn't in Wikidata, like adding sound properties or some other grammemes. The filter properties there does reorder, remove, and merge some lexemes. It's all automated. No manual edits were performed. The sound properties could be included into Wikidata by adding phonetic information to many words. That's hard to derive when you only need a subset of the data. So right now, that data is primarily separate. If Wikidata has phonetic information on a form, it will be parsed and the tool will derive the important information. |
I recommend reviewing The |
01e0926 to
41b1ac4
Compare
Inflection-187 Support SemanticConcept in icu::message2 for custom vocabulary Inflection-186 Support the print and speak lines from SpeakableString in icu::message2 Inflection-185 Support lists in icu::message2 Inflection-183 Support quantify in icu::message2 Inflection-178 Provide quantity support for Serbian Inflection-117 Support inflection functionality into icu::message2 Inflection-113 Derive sound properties from Wikidata for Korean and Turkish Inflection-87 Integrate Unicode Inflection into Unicode Message Format Support more languages
Fixes #188
Fixes #187
Fixes #186
Fixes #185
Fixes #183
Fixes #178
Fixes #117
Fixes #113
Fixes #87
The changes address the following issues.
The version of ICU needs to be updated before the pull request checks can pass. The Language List should be updated once this pull request is merged.