Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[voice] Reduce collisions on exact match and use item synonyms #3698

Merged
merged 8 commits into from
Sep 8, 2023

Conversation

GiviMAD
Copy link
Member

@GiviMAD GiviMAD commented Jul 10, 2023

Hi openHAB maintainers, I would like to add this changes to the StandardInterpreter so it uses the item synonyms and to reduce collision between items labels in case of single exact match. I think the changes can be seen as a bug fix as the current item resolution method is unchanged an takes priority in case of succeed.

Note that I have added a test suite for the StandardInterpreter with tests cases for the changes included and also some debug logs to the AbstractRuleBasedInterpreter getMatchingItems method so users have a way to know what the collisions are.

Hope everything is in place.

Best regards

@GiviMAD GiviMAD requested a review from a team as a code owner July 10, 2023 19:50
GiviMAD added 3 commits July 10, 2023 22:30
Signed-off-by: Miguel Álvarez <[email protected]>
Signed-off-by: Miguel Álvarez <[email protected]>
@GiviMAD
Copy link
Member Author

GiviMAD commented Jul 11, 2023

I was testing the proposed changes at my home over the last snapshot and realize those didn't work well with groups, I have to introduce more changes to fix my problems.

I think with the latest changes name collisions are better handled. With that and the item synonyms seems like I can fine tune how it works without so much trouble.

For example now I can have multiple groups items whose labels collides like "Big Lamp" and "Small Lamp" and by adding a synonym to one them control what is the target when I use the command "turn off lamp" instead of getting an error saying two or more items have similar names.

And the debug logs help a lot to understand the collision problems.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Code looks good but is very difficult to understand due to such undocumented object Map<Locale, Map<Item, List<List<List<String>>>>> itemTokens !

@GiviMAD
Copy link
Member Author

GiviMAD commented Jul 14, 2023

Code looks good but is very difficult to understand due to such undocumented object Map<Locale, Map<Item, List<List<List>>>> itemTokens !

True.

Having a group item labeled as "Samsung TV" with synonym "Smart TV" which contains a switch item labeled as "Power" the object will look as:

{
	localeRef: {
		groupItemRef: [
			[ ["Samsung", "TV"] ],
			[ ["Smart", "TV"] ],
		],
		switchItemRef: [
			[ ["Power"] ],
			[ ["Samsung", "TV"], ["Power"] ],
			[ ["Smart", "TV"], ["Power"] ],
		]
	}
}

The first list level are the alternative paths to the item, the second level is a path to the item conformed by collections of tokens for the different parents and itself.

Not sure what is the best way of document this, maybe I can add that example to the getItemTokens comment (which is outdated after my changes, I didn't realize before).

Signed-off-by: Miguel Álvarez <[email protected]>
@GiviMAD
Copy link
Member Author

GiviMAD commented Jul 14, 2023

I have updated the method comment with the current behavior.

@GiviMAD GiviMAD requested a review from lolodomo July 14, 2023 16:09
List<String> unmatchedFragments = new ArrayList<>(lowercaseLabelFragments);
for (List<String> segment : parts) {
if (segment.equals(lowercaseLabelFragments)) {
exactMatch = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why is that important? And I believe this is not true in general:

  • labelFragments [a, b, c, d]
  • segments: [[b], [c, d], [a]]
  • first segment, there is [a, c, d] left
  • second segment, there is [a] left
  • third segment matches everything that is left, so exactMatch = true

Is that intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the review, and sorry for taking so long to answer, it caught me in the middle of another task that took more than expected.

Maybe I don't understanding the question but I think the code ok.
Each segment is compared against lowercaseLabelFragments which is never modified, checking if some segment in the path is an exact match agains the label extracted from the voice command. Each iteration I'm trying to remove from unmatchedFragments but that is a different list initialized with same strings.

I have renamed the segment and parts variables trying to make that part easier to read.

Signed-off-by: Miguel Álvarez <[email protected]>
@GiviMAD GiviMAD requested a review from J-N-K September 7, 2023 20:51
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Thanks.

@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Sep 8, 2023
@J-N-K J-N-K added this to the 4.1 milestone Sep 8, 2023
@J-N-K J-N-K merged commit 4364e3b into openhab:main Sep 8, 2023
2 checks passed
@GiviMAD
Copy link
Member Author

GiviMAD commented Sep 9, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants