Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions inflection/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,7 @@ file(MAKE_DIRECTORY ${INFLECTION_INCLUDE_ROOT})

include(dependICU)

add_library(xml2 INTERFACE IMPORTED GLOBAL)
set_target_properties(xml2 PROPERTIES IMPORTED_LIBNAME xml2)
target_include_directories(xml2 INTERFACE ${CMAKE_OSX_SYSROOT}/usr/include/libxml2)
find_package(LibXml2 REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good improvement.


# Runs Unicode Inflection unit tests: "make check"
set(DYLD_LIBRARY_PATH ${ICU_LIB_DIRECTORY}:$<TARGET_FILE_DIR:inflection>)
Expand Down
2 changes: 0 additions & 2 deletions inflection/src/inflection/dialog/DictionaryLookupFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@
*/
#include <inflection/npc.hpp>
#include <inflection/dialog/DictionaryLookupFunction.hpp>

#include <inflection/dialog/DisplayValue.hpp>
#include <inflection/dialog/SpeakableString.hpp>
#include <inflection/dictionary/DictionaryMetaData.hpp>
#include <inflection/util/Validate.hpp>
#include "inflection/tokenizer/TokenChain.hpp"
#include "inflection/tokenizer/Token_Word.hpp"
#include "inflection/tokenizer/TokenizerFactory.hpp"

namespace inflection::dialog {

DictionaryLookupFunction::DictionaryLookupFunction(const ::inflection::util::ULocale& locale, const ::std::vector<::std::u16string>& tags)
Expand Down
23 changes: 19 additions & 4 deletions inflection/src/inflection/dialog/DisplayValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@
namespace inflection::dialog {


DisplayValue::DisplayValue(const ::std::u16string& displayString, const ::std::map<SemanticFeature, ::std::u16string>& constraintMap)
DisplayValue::DisplayValue(
const ::std::u16string& displayString,
const ::std::map<SemanticFeature, ::std::u16string>& constraintMap
)
: super()
, displayString(displayString)
, constraintMap(constraintMap)
Expand All @@ -21,11 +24,23 @@ DisplayValue::DisplayValue(const ::std::u16string& value)
{
}

DisplayValue::DisplayValue(const SpeakableString& value, const SemanticFeature& speakFeature)
: DisplayValue(value.getPrint(), {})
DisplayValue::DisplayValue(
const SpeakableString& value,
const SemanticFeature& speakFeature
)
: DisplayValue(value, speakFeature, {})
{
}

DisplayValue::DisplayValue(
const SpeakableString& value,
const SemanticFeature& speakFeature,
const ::std::map<SemanticFeature, ::std::u16string>& constraintMap
)
: DisplayValue(value.getPrint(), constraintMap)
{
if (!value.speakEqualsPrint()) {
constraintMap.emplace(speakFeature, value.getSpeak());
this->constraintMap.emplace(speakFeature, value.getSpeak());
}
}

Expand Down
7 changes: 7 additions & 0 deletions inflection/src/inflection/dialog/DisplayValue.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,13 @@ class INFLECTION_CLASS_API inflection::dialog::DisplayValue
* @param speakFeature The speakFeature from the SemanticFeatureModel that represents the SemanticFeature for the speak information for a SpeakableString.
*/
DisplayValue(const SpeakableString& value, const SemanticFeature& speakFeature);
/**
* Construct a display value with a SpeakableString.
* @param value A SpeakableString
* @param speakFeature The speakFeature from the SemanticFeatureModel that represents the SemanticFeature for the speak information for a SpeakableString.
* @param constraintMap The intitial constraint map.
*/
DisplayValue(const SpeakableString& value, const SemanticFeature& speakFeature, const ::std::map<SemanticFeature, ::std::u16string>& constraintMap);
/**
* The destructor
*/
Expand Down
27 changes: 26 additions & 1 deletion inflection/src/inflection/dialog/InflectableStringConcept-c.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,18 @@
* Copyright 2021-2024 Apple Inc. All rights reserved.
*/
#include <inflection/dialog/InflectableStringConcept.h>

#include <inflection/npc.hpp>
#include <inflection/dialog/InflectableStringConcept.hpp>
#include <inflection/dialog/SemanticFeatureConcept.h>
#include <inflection/exception/ClassCastException.hpp>
#include <inflection/util/ULocale.hpp>
#include <inflection/dialog/SemanticFeature.hpp>
#include <inflection/dialog/SemanticUtils.hpp>
#include <inflection/dialog/SemanticFeatureModel.hpp>
#include <inflection/util/TypeConversionUtils.hpp>
#include <inflection/util/Validate.hpp>


INFLECTION_CAPI IDSemanticFeatureConcept* iinf_toSemanticFeatureConcept(IDInflectableStringConcept* thisObject, UErrorCode*)
{
return (IDSemanticFeatureConcept*)thisObject;
Expand Down Expand Up @@ -47,6 +51,27 @@ iinf_create(const IDSemanticFeatureModel* model, const IDSpeakableString* value,
return nullptr;
}

INFLECTION_CAPI IDInflectableStringConcept*
iinf_createWithDefaults(const IDSemanticFeatureModel* model, const IDSpeakableString* value,
const IDDisplayValue_Constraint* defaultConstraints, int32_t defaultConstraintsLen, UErrorCode* status)
{
if (status != nullptr && U_SUCCESS(*status)) {
try {
auto defaultConstraintsMap(inflection::dialog::SemanticUtils::to_constraintMap(*npc((const inflection::dialog::SemanticFeatureModel*)model), defaultConstraints, defaultConstraintsLen));

return (IDInflectableStringConcept*) new ::inflection::dialog::InflectableStringConcept(
(const ::inflection::dialog::SemanticFeatureModel*)model,
*((const ::inflection::dialog::SpeakableString*)value),
defaultConstraintsMap
);
}
catch (const ::std::exception& e) {
inflection::util::TypeConversionUtils::convert(e, status);
}
}
return nullptr;
}

INFLECTION_CAPI void
iinf_destroy(IDInflectableStringConcept* thisObject)
{
Expand Down
58 changes: 48 additions & 10 deletions inflection/src/inflection/dialog/InflectableStringConcept.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,23 @@

namespace inflection::dialog {

InflectableStringConcept::InflectableStringConcept(const SemanticFeatureModel* model, const SpeakableString& value)

InflectableStringConcept::InflectableStringConcept(
const SemanticFeatureModel* model,
const SpeakableString& value
)
: InflectableStringConcept(model, value, {})
{
}

InflectableStringConcept::InflectableStringConcept(
const SemanticFeatureModel* model,
const SpeakableString& value,
const ::std::map<SemanticFeature, ::std::u16string>& intitialConstraints
)
: super(model)
, value(value)
, defaultDisplayValue(value, *npc(super::getSpeakFeature()))
, defaultDisplayValue(value, *npc(super::getSpeakFeature()), intitialConstraints)
{
}

Expand All @@ -39,11 +52,40 @@ SpeakableString* InflectableStringConcept::getFeatureValue(const SemanticFeature
if (constraint != nullptr) {
return new SpeakableString(*npc(constraint));
}
const auto displayValueResult = getDisplayValue(true);
if (displayValueResult) {
const auto defaultFeatureFunction = npc(getModel())->getDefaultFeatureFunction(feature);
::std::unique_ptr<SpeakableString> computedValue;
::std::unique_ptr<SpeakableString> baseValue;
if (defaultFeatureFunction != nullptr) {
computedValue.reset(npc(defaultFeatureFunction)->getFeatureValue(*displayValueResult, constraints));
baseValue.reset(npc(defaultFeatureFunction)->getFeatureValue(defaultDisplayValue, constraints));
}
const auto& displayConstraintMap = displayValueResult->getConstraintMap();
auto displayConstraint = displayConstraintMap.find(feature);
if (displayConstraint != displayConstraintMap.end()) {
auto numberFeature = npc(getModel())->getFeature(u"number");
if (feature.getName() == u"gender" && baseValue && numberFeature != nullptr && displayConstraintMap.find(*npc(numberFeature)) != displayConstraintMap.end()) {
return baseValue.release();
}
return new SpeakableString(displayConstraint->second);
}
if (computedValue) {
return computedValue.release();
}
if (baseValue) {
return baseValue.release();
}
}
const auto& initialConstraintMap = defaultDisplayValue.getConstraintMap();
auto initialConstraint = initialConstraintMap.find(feature);
if (initialConstraint != initialConstraintMap.end()) {
return new SpeakableString(initialConstraint->second);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are confusing. Why are these changes needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More precisely, why is getFeatureValue being called here? Why is the feature name being checked for gender? What's special about gender or number? Why not grammatical case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto displayConstraint = displayConstraintMap.find(feature);
if (displayConstraint != displayConstraintMap.end()) {
    auto numberFeature = npc(getModel())->getFeature(u"number");
    if (feature.getName() == u"gender" && baseValue && numberFeature != nullptr
        && displayConstraintMap.find(*npc(numberFeature)) != displayConstraintMap.end()) {
        return baseValue.release();
    }
    return new SpeakableString(displayConstraint->second);
} 

This is the test case which was failing before the changes:

<test>
  <source definiteness="definite">cometa</source>
  <initial gender="masculine"/>
  <result gender="masculine" number="singular">el cometa</result>
</test>
  • InflectableStringConcept::getFeatureValue asks the default display function for a rendered DisplayValue, so feature lookups reflect whatever the display layer injected (articles, inflections, etc.). Spanish determiners add both gender and number to that map, and the gender can contradict the lexeme default (la cometa).

  • To avoid reporting the article’s guess as the noun’s gender, the code detects feature == "gender" and, when the number constraint is present too, returns the lexeme’s base value instead of the display-layer value. English articles never set gender, and no language mutates case in this path yet (I am not sure though), so only the gender/number pair needs a guard today.

  • InflectableStringConcept::getFeatureValue asks the default display function for a rendered DisplayValue, so feature lookups reflect whatever the display layer injected (articles, inflections, etc.).

  • Spanish determiners add both gender and number to that map, and the gender can contradict the lexeme default (la cometa).
    To avoid reporting the article’s guess as the noun’s gender, the code detects feature == "gender" and, when the number constraint is present too, returns the lexeme’s base value instead of the display-layer value, as these conditions when satisfied point to the case when we have an article being inserted.

  • Grammatical case isn’t treated specially because the Spanish article logic never synthesizes case information—so there’s no regression to guard against. If a language later injected case-altering determiners we’d need a similar safeguard, but today only the gender/number pair exhibits the mismatch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior does not scale, and it does not make sense in my mind. An InflectableStringConcept is just a simplified SemanticConcept. A SemanticConcept should not have this issue. It does not have this kind of logic, and it does not check on specific grammatical category values.

auto defaultFeatureFunction = npc(getModel())->getDefaultFeatureFunction(feature);
if (defaultFeatureFunction != nullptr) {
const auto displayValueResult = getDisplayValue(true);
if (displayValueResult) {
return npc(defaultFeatureFunction)->getFeatureValue(*displayValueResult, constraints);
if (auto fallbackValue = ::std::unique_ptr<SpeakableString>(npc(defaultFeatureFunction)->getFeatureValue(defaultDisplayValue, constraints))) {
return fallbackValue.release();
}
}
return nullptr;
Expand All @@ -63,11 +105,7 @@ ::std::optional<DisplayValue> InflectableStringConcept::getDisplayValue(bool all
{
auto defaultDisplayFunction = npc(getModel())->getDefaultDisplayFunction();
if (defaultDisplayFunction != nullptr && !constraints.empty()) {
::std::map<SemanticFeature, ::std::u16string> constraintMap;
if (!value.speakEqualsPrint()) {
constraintMap.emplace(*npc(getSpeakFeature()), value.getSpeak());
}
SemanticFeatureModel_DisplayData displayData({DisplayValue(value.getPrint(), constraintMap)});
SemanticFeatureModel_DisplayData displayData({defaultDisplayValue});
::std::unique_ptr<DisplayValue> returnVal(npc(defaultDisplayFunction)->getDisplayValue(displayData, constraints, allowInflectionGuess));
if (returnVal != nullptr) {
return *returnVal;
Expand Down
15 changes: 14 additions & 1 deletion inflection/src/inflection/dialog/InflectableStringConcept.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#include <inflection/dialog/SpeakableString.h>
#include <inflection/dialog/SemanticFeatureConcept.h>
#include <inflection/dialog/SemanticFeatureModel.h>

#include <inflection/dialog/DisplayValue.h>
/**
* An object that provides a way to format a word with additional grammatical category values or semantic features of a word for a given language.
*/
Expand Down Expand Up @@ -34,6 +34,19 @@ INFLECTION_CAPI IDInflectableStringConcept* iinf_toInflectableStringConcept(IDSe
* This is set to a failure when a failure has occurred during execution.
*/
INFLECTION_CAPI IDInflectableStringConcept* iinf_create(const IDSemanticFeatureModel* model, const IDSpeakableString* value, UErrorCode* status);
/**
* Constructs a concept given a semantic feature model and a speakable string
*
* @param model - The semantic feature model required to initialize the concept.
* @param value - The speakable string to convert to a concept
* @param defaultConstraints - The initial defaultConstraints to apply to the concept
* @param defaultConstraintsLen - The number of semantic features and values provided
* @param status Must be a valid pointer to an error code value,
* which must not indicate a failure before the function call.
* This is set to a failure when a failure has occurred during execution.
*/
INFLECTION_CAPI IDInflectableStringConcept* iinf_createWithDefaults(const IDSemanticFeatureModel* model, const IDSpeakableString* value,
const IDDisplayValue_Constraint* defaultConstraints, int32_t defaultConstraintsLen, UErrorCode* status);
/**
* Destructor
*/
Expand Down
9 changes: 9 additions & 0 deletions inflection/src/inflection/dialog/InflectableStringConcept.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,15 @@ class INFLECTION_CLASS_API inflection::dialog::InflectableStringConcept
* @param value - The speakable string to convert to a concept
*/
InflectableStringConcept(const SemanticFeatureModel* model, const SpeakableString& value);

/**
* Constructs a concept given a semantic feature model and a speakable string
*
* @param model - The semantic feature model required to initialize the concept.
* @param value - The speakable string to convert to a concept
* @param intitialConstraints - The intitial constraints for the map.
*/
InflectableStringConcept(const SemanticFeatureModel* model, const SpeakableString& value, const std::map<SemanticFeature, ::std::u16string>& intitialConstraints);
/**
* Copy constructor
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ ::std::optional<::std::u16string> FrGrammarSynthesizer_CountLookupFunction::dete
int64_t wordGrammemes = 0;
dictionary.getCombinedBinaryType(&wordGrammemes, word);
if ((wordGrammemes & properNounProperty) == properNounProperty) {
return {{}};
return std::u16string{};
}
if (checkInvariantNouns(word, wordGrammemes)) {
return GrammemeConstants::NUMBER_SINGULAR();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ ::std::optional<::std::u16string> HiGrammarSynthesizer_HiDisplayFunction::inflec

::std::optional<::std::vector<::std::u16string>> HiGrammarSynthesizer_HiDisplayFunction::inflectSignificantWords(const std::vector<::std::u16string> &words, const ::std::map<SemanticFeature, ::std::u16string> &constraints, bool enableInflectionGuess) const {
if (words.empty()) {
return {{}};
return std::vector<std::u16string>{};
}
const auto &dictionary = dictionaryInflector.getDictionary();
int64_t adpositionIndex = -1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ ::std::optional<::std::vector<::std::u16string>> PtGrammarSynthesizer_PtDisplayF

::std::optional<::std::vector<::std::u16string>> PtGrammarSynthesizer_PtDisplayFunction::inflectSignificantWords(const std::vector<::std::u16string> &words, const ::std::map<::inflection::dialog::SemanticFeature, ::std::u16string> &constraints, bool enableInflectionGuess) const {
switch (words.size()) {
case 0: return {{}};
case 0: return std::vector<std::u16string>{};
case 1: {
int64_t wordType = 0;
dictionary.getCombinedBinaryType(&wordType, words[0]);
Expand Down
2 changes: 1 addition & 1 deletion inflection/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ target_link_libraries(itest
PRIVATE
inflection
Catch2
xml2
LibXml2::LibXml2
marisa_objs
ICU::uc ICU::i18n
$<$<PLATFORM_ID:Darwin>:${PERFDATA_FRAMEWORK}>
Expand Down
7 changes: 7 additions & 0 deletions inflection/test/resources/inflection/dialog/inflection/es.xml
Original file line number Diff line number Diff line change
Expand Up @@ -221,4 +221,11 @@
<test><source definiteness="definite">álgebra añeja</source><result>el álgebra añeja</result></test> <!-- An exception to the gender rule. -->
<test><source definiteness="definite">añeja álgebra</source><result>la añeja álgebra</result></test>
<test><source>área</source><result withDemAdj="esta área"/></test>

<!-- New Tests for stating grammemes -->
<test><source definiteness="definite">cometa</source><initial gender="masculine"/><result number="singular" gender="masculine">el cometa</result></test>
<test><source definiteness="definite">cometa</source><initial gender="feminine"/><result number="singular" gender="feminine">la cometa</result></test>
<test><source definiteness="definite">QQQQ</source><initial gender="masculine" number="plural"/><result number="plural" gender="masculine">los QQQQ</result></test>
<test><source definiteness="definite">QQQQ</source><initial gender="feminine" number="plural"/><result number="plural" gender="masculine">las QQQQ</result></test>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests look reasonable.


</inflectionTest>
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,73 @@ TEST_CASE("InflectableStringConceptTest-c#testExistsAPISpanish")
iinf_destroy(inflectableConcept);
}

TEST_CASE("InflectableStringConceptTest-c#testCreateWithDefaults")
{
auto error = U_ZERO_ERROR;
auto ccfp = ilccfp_getDefaultCommonConceptFactoryProvider(&error);
REQUIRE(U_SUCCESS(error));

const auto locale = ::inflection::util::LocaleUtils::SPANISH().getName().c_str();
auto ccf = ilccfp_getCommonConceptFactory(ccfp, locale, &error);
REQUIRE(U_SUCCESS(error));

auto model = iccf_getSemanticFeatureModel(ccf, &error);
REQUIRE(U_SUCCESS(error));

const std::u16string source = u"cometa";
auto sourceString = iss_create(source.c_str(), static_cast<int32_t>(source.size()), &error);
REQUIRE(U_SUCCESS(error));

IDDisplayValue_Constraint defaultConstraints[] = {
{u"gender", u"masculine"},
};

auto inflectableConcept = iinf_createWithDefaults(model,
sourceString,
defaultConstraints,
static_cast<int32_t>(sizeof(defaultConstraints) / sizeof(defaultConstraints[0])),
&error);
iss_destroy(sourceString);
REQUIRE(U_SUCCESS(error));
REQUIRE(inflectableConcept != nullptr);

auto semanticConcept = iinf_toSemanticFeatureConcept(inflectableConcept, &error);
REQUIRE(U_SUCCESS(error));

isfc_putConstraintByName(semanticConcept, u"definiteness", u"definite", -1, &error);
REQUIRE(U_SUCCESS(error));

auto printedValue = isfc_toSpeakableStringCopy(semanticConcept, &error);
REQUIRE(U_SUCCESS(error));
util::StringContainer<IDSpeakableString, iss_getPrint> printedContainer(printedValue);
REQUIRE(printedContainer);
CHECK(printedContainer.value == u"el cometa");
iss_destroy(printedValue);

auto genderValue = isfc_createFeatureValueByNameCopy(semanticConcept, u"gender", &error);
REQUIRE(U_SUCCESS(error));
util::StringContainer<IDSpeakableString, iss_getPrint> genderContainer(genderValue);
REQUIRE(genderContainer);
CHECK(genderContainer.value == u"masculine");
iss_destroy(genderValue);

auto numberValue = isfc_createFeatureValueByNameCopy(semanticConcept, u"number", &error);
REQUIRE(U_SUCCESS(error));
util::StringContainer<IDSpeakableString, iss_getPrint> numberContainer(numberValue);
REQUIRE(numberContainer);
CHECK(numberContainer.value == u"singular");
iss_destroy(numberValue);

auto definitenessValue = isfc_createFeatureValueByNameCopy(semanticConcept, u"definiteness", &error);
REQUIRE(U_SUCCESS(error));
util::StringContainer<IDSpeakableString, iss_getPrint> definitenessContainer(definitenessValue);
REQUIRE(definitenessContainer);
CHECK(definitenessContainer.value == u"definite");
iss_destroy(definitenessValue);

iinf_destroy(inflectableConcept);
}

TEST_CASE("InflectableStringConceptTest-c#testSpanish")
{
auto error = U_ZERO_ERROR;
Expand Down
Loading
Loading