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

media type #32

Closed
wants to merge 9 commits into from
Closed

Conversation

JarbasAl
Copy link
Contributor

@JarbasAl JarbasAl commented Aug 13, 2020

possible solution for MycroftAI/mycroft-core#2658

companion PR MycroftAI/mycroft-core#2660

NOTE: this will break lang support except for en-us, this is because of migration from adapt to padatious to get rid of the ugly regex hack

To test
install https://github.com/JarbasSkills/skill-hppodcraft/tree/cps_mediatype and verify

  • "read the call of cthulhu" uses common play with media type audiobook,
  • "play lovecraft podcast" should use podcast media type
  • "play {story_name}" should trigger with generic media type
  • "read lovecraft" should tie with skill-epic-horror-theatre if using CPS ties #36

install https://github.com/JarbasSkills/skil-old-world-radio/tree/cps_media_type and verify

  • only triggers with media type radio
  • "play vintage radio", "play old world radio"
  • "play some radio" should trigger with low confidence

install MycroftAI/skill-npr-news#90

  • verify "play the news" triggers default feed with type news
  • verify "play {station_name} news" triggers news station with type news
  • "play {station_name}" should trigger with type generic

install https://github.com/JarbasSkills/skill-epic-horror-theatre/tree/feat/cps_media

  • "read innsmouth" uses common play with media type audiobook,
  • "start audio drama" low confidence with media type audiobook,
  • "play the mountains of madness" should trigger with generic media type
  • "read lovecraft" should tie with skill-hppodcraft if using CPS ties #36

@JarbasAl
Copy link
Contributor Author

JarbasAl commented Aug 18, 2020

relates to #18

"play music" with this PR becomes media type MUSIC and phrase is "" empty string

@krisgesling
Copy link
Contributor

Hey this looks really good, and thanks for the many test cases to stretch its legs.

In terms of breaking language support, would there be an issue with leaving the Adapt handler in until the next major release? The new Padatious intent should match with higher confidence unless the intent files don't exist for a language, then it should fallback to the old Adapt intent.

@krisgesling
Copy link
Contributor

Hey just saw this again, what do you think of my comment above?

I'm hesitant to break all Play intents for anyone not using "en-us"

@JarbasAl
Copy link
Contributor Author

im thinking that might cause conflicts, adapt is "privileged" when matching intents, a padatious intent with 80% confidence in the case of this skill would still be pretty accurate but overrided, adapt essentially overmatches. due to the nature of capture groups the confidence of padatious might not be enough for proper selection

i think as a transition step we could register adapt only for other languages? not sure how well this fits in translate platform

A bigger issue (need to confirm this) is that trying to load the padatious intent files throw an error anyway if the files are missing for the configured language?

@krisgesling
Copy link
Contributor

Yeah Padatious requires at least 95% confidence to beat Adapt so the training examples need to be pretty well defined.

If we can make a work around to register Adapt for certain languages that would be fine.

Is that an error if the language directory exists but the file doesn't?

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.

None yet

2 participants