Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Adapt intent matches not returned to core in confidence order. #2947

Closed
wants to merge 1 commit into from
Closed

Adapt intent matches not returned to core in confidence order. #2947

wants to merge 1 commit into from

Conversation

chrisveilleux
Copy link
Member

Description

The AdaptService in core assumes that the highest confidence intent match will be the first result returned from Adapt. This is not always the case. It is possible that Adapt should be ordering the return value instead. But that would be a bigger change to Adapt.

NOTE: this replaces PR2915

How to test

  • Remove the future time Padatious intent from the date/time skill
  • Ask "what time will it be in ten minutes"; current time will be returned.
  • Apply the change in this PR
  • Make same request as before, future time will be returned.

Contributor license agreement signed?

CLA [Y]

@pep8speaks
Copy link

pep8speaks commented Jul 8, 2021 •
edited
Loading

Hello @chrisveilleux! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-08 19:31:10 UTC

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jul 8, 2021
@JarbasAl
Copy link
Contributor

JarbasAl commented Jul 8, 2021

isnt this handled by MycroftAI/adapt#137 ?

merging that and bumping adapt seems like a better fix no?

@chrisveilleux
Copy link
Member Author

isnt this handled by MycroftAI/adapt#137 ?

merging that and bumping adapt seems like a better fix no?

I am not entirely sure the code in that PR fixes this issue.

@chrisveilleux chrisveilleux force-pushed the bugfix/intent-confidence-order branch from 7a5b51a to f07c80f Compare July 8, 2021 19:31
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2021 •
edited
Loading

Codecov Report

Merging #2947 (7a5b51a) into dev (b3a0b3b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7a5b51a differs from pull request most recent head f07c80f. Consider uploading reports for the commit f07c80f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #2947      +/-   ##
==========================================
- Coverage   52.65%   52.64%   -0.01%     
==========================================
  Files         123      123              
  Lines       11023    11024       +1     
==========================================
  Hits         5804     5804              
- Misses       5219     5220       +1
Impacted Files Coverage Δ
mycroft/skills/intent_services/adapt_service.py 80.71% <100.00%> (+0.13%) ⬆️
mycroft/client/speech/listener.py 48.04% <0.00%> (-0.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3a0b3b...f07c80f. Read the comment docs.

@clusterfudge
Copy link
Contributor

isnt this handled by MycroftAI/adapt#137 ?
merging that and bumping adapt seems like a better fix no?

I am not entirely sure the code in that PR fixes this issue.

@chrisveilleux can you clarify what edge cases you think MycroftAI/adapt#137 does not address? happy to add tests to confirm/fix.

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

This was referenced Jul 8, 2021
Merged
Merged
@chrisveilleux
Copy link
Member Author

isnt this handled by MycroftAI/adapt#137 ?
merging that and bumping adapt seems like a better fix no?

I am not entirely sure the code in that PR fixes this issue.

@chrisveilleux can you clarify what edge cases you think MycroftAI/adapt#137 does not address? happy to add tests to confirm/fix.

I will try to give an example in the Adapt PR.

@chrisveilleux
Copy link
Member Author

Closing this PR. I believe the issue was fixed with Adapt PR 128. The parse confidence passed to Intent.validate_with_tags() was overridden in the method, causing an incorrect value to be used in the final confidence calculation.

I tested the scenario that caused me to generate this PR and it is no longer an issue.

A great example of why you should never mess with the value of a parameter in a method or function.

@chrisveilleux chrisveilleux deleted the bugfix/intent-confidence-order branch July 10, 2021 01:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JarbasAl JarbasAl JarbasAl left review comments

@forslund forslund Awaiting requested review from forslund

@krisgesling krisgesling Awaiting requested review from krisgesling

@ken-mycroft ken-mycroft Awaiting requested review from ken-mycroft

Assignees
No one assigned
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chrisveilleux @pep8speaks @JarbasAl @codecov-commenter @clusterfudge @devops-mycroft