When multiple entities match, only one is returned

[hallman76]

Hey folks - incredible project here!

I bumped across an issue in my testing and wanted to provide a patch. Am opening an issue to refer to in the commit. If I've overlooked something obvious or if I'm approaching this the wrong way, please point me in the right direction.

When determining intent it's possible that more than one entity will provide a match. Currently, Adapt returns the first match. It should provide a list of all matches.

I ran into this when creating a simple test intent locate my friends. It matched on a command like "Where is Bob?"

engine = IntentDeterminationEngine()

for keyword in ['locate', 'where']:
    engine.register_entity(keyword, "LocateKeyword")

for name in ["Bob", "Smith", "Bob Smith"]:
    engine.register_entity(name, "Person", alias_of='bsmith')

for name in ["Bob", "Jones", "Bob Jones"]:
    engine.register_entity(name, "Person", alias_of='bjones')

for name in ["Mike", "Jones", "Mike Jones"]:
    engine.register_entity(name, "Person", alias_of='mjones')

locate_person_intent = IntentBuilder("LocatePersonIntent")\
    .require("LocateKeyword")\
    .require("Person")\
    .build()

engine.register_intent_parser(locate_person_intent)

Searching for our friend Bob will find the correct intent, but it will only reference one of the two Bobs.

>>> for intent in engine.determine_intent("Where is Bob?", num_results=5):
>>>     print(intent)
{'confidence': 1.0, 'Person': 'bjones', 'target': None, 'intent_type': 'LocatePersonIntent', 'LocateKeyword': 'where'}

I think it would be helpful if it returned a list of all matches:

{'confidence': 1.0, 'Person': ['bjones', 'bsmith'], 'target': None, 'intent_type': 'LocatePersonIntent', 'LocateKeyword': 'where'}

note, this is different than the suggestion in issue #42

[aatchison]

Awesome, thanks!

[randomstuff]

This would break existing callers, expecting a string in Person. Wouldn't it be better to opt-in with something such as .require("Person", list=True):

• list would default to False: return at most one value, or None for .optionally();
• list could be set to True: always return a list, which might by empty for .optionally()

[clusterfudge]

2 years later...

I'd actually discourage the example use-case of adapt. The parser isn't really intended to be used like a search index, and a skill (or library) that's capable of understanding contact search would be a better implementation. In this way, the intent would look like

locate_person_intent = IntentBuilder("LocatePersonIntent")\
    .require("LocateKeyword")\
    .one_of("Person", "ContactSearchQuery")\
    .build()

where ContactSearchQuery is a regex entity. You could also implement that as two separate intents, which might result in your skill implementation being a bit cleaner.

As for the List entity type (as suggested by @randomstuff), I think this would be a wonderful addition! I'm a little on the fence as to how to implement it, but it's definitely a reasonable suggestion!

Reading a little more closely, it seems like what I'm saying is that I like #42 :)