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

Change the location.rx files to be less greedy. They were conflictin… #176

Open
wants to merge 1 commit into
base: 21.02
Choose a base branch
from

Conversation

chrisveilleux
Copy link
Member

Description

Change the location.rx files to be less greedy. They were conflicting with the location.rx files in other skills.

If needed follow up with as much detail as required.

Type of PR

If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR.
Either delete those that do not apply, or add an x between the square brackets like so: - [x]

  • Bugfix
  • Feature implementation
  • Refactor of code (without functional changes)
  • Documentation improvements
  • Test improvements

Testing

This change should be a no-op for this skill so VK tests should pass.

Documentation

Not needed for this PR

… with the location.rx files in other skills.
@krisgesling
Copy link
Contributor

With this change the VK tests aren't passing for me.

The description somewhat concerns me though... Does this mean that if I load another Skill that has a location.rx file registered with Adapt that it might impact the behaviour of the location regex in this Skill?

If so, has this been raised as an issue in the Adapt repo already? It seems very problematic.

@forslund
Copy link
Collaborator

forslund commented Aug 4, 2021

The issue here is the .* in the beginning will cause it to generate a lot of extra matches making the regex parsing in adapt heavier and giving lower scores. it should not be there, so to me this is definitely a good change if the VK tests can be made to pass...

I also think the * at the end in *.+ can be removed as well since it will likely always be matched against the empty sequence (If I'm reading the regex correctly, which is definitely not certain)

General rule for adapt regexes In general .+ (match at least one any character) is preferable over .* (match at least zero any character)

@krisgesling
Copy link
Contributor

krisgesling commented Aug 5, 2021

I'm getting much more consistent behaviour with the recent changes. Though this might be false optimism...

I am still getting an issue with the test for these two utterances:

temperature in the afternoon
what is the weather in London later today

and it seems to be due to this location regex.

At a more foundational level, it seems like a literal keyword match should be given more weight than a patterned regex match. Eg afternoon.voc should get a higher confidence level than this location regex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants