[EN] Extended light set sentences #3140
Conversation
…<brightness_synonyms>' that allows for synonyms to brightness like 'light' or 'lights'. Removed duplicate sentances from tests.
There was a problem hiding this comment.
Hi @onedashone
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
@mrdarrengriffin @ViViDboarder please double check |
sentences/en/_common.yaml
Outdated
| @@ -376,6 +376,7 @@ expansion_rules: | |||
| are_any: "[<are>] <any>" | |||
| how_many: "how many [of <the>]" | |||
| brightness: "{brightness}[([ ]%)| percent]" | |||
| brightness_synonyms: "(brightness|light[s])" | |||
There was a problem hiding this comment.
Wouldn't it be better to use the expansion rule for lights here?
There was a problem hiding this comment.
This excellent question led me down a much longer path than I originally anticipated. In short, swapping out "light[s]" with "<light>" better aligns with the current setup for the light_HassTurnOff and light_HassTurnOn sentence structures. However, once I started adding some additional tests to cover the additions "<light>" would create, I quickly realized that simply swapping out "brightness" with "<brightness_synonyms>" wasn't a good long term solution.
My new commit tries to clearly differentiate between words that define general light entities and words used as a measurement of light like brightness.
|
@mrdarrengriffin @ViViDboarder @tetele I did a pretty big overhaul to the branch that probably warrants a review. If my changes cause too many concerns, I'll revert to my last commit and simply implement mrdarrengriffin's suggestion. Here's a quick rundown of the changes:
The changes to sentence structure comes down to this:
|
|
Just realized the update to skip_words caused a few tests outside of light_HassLightSet to fail. Should be pretty easy to fix if it's anything like the change I had to make for light_HassLightSet. I'll look into that soon. |
sentences/en/light_HassLightSet.yaml
Outdated
| - "[<numeric_value_set>] <name> brightness [to] <brightness>" | ||
| - "[<numeric_value_set>] [the] brightness [of] <name> [to] <brightness>" | ||
| - "[<numeric_value_set>] <name> [to] <brightness> [brightness]" | ||
| - "[<numeric_value_set>] ([of] <name>;<light_value>;[[the] <brightness_synonyms>])" |
There was a problem hiding this comment.
Although i am a fan of contraction, this template is excessive. I'll underline the issues.
- In a permutation, having a completely optional member is a no-no. The reason for that is that there are too many generated variants which would match.
As an example, (a;b;[c]) expands into 6 variants: a b [c], a [c] b, b a [c], b [c] a, [c] a b and [c] b a. Of those, when c is missing, 3 variants will match a b: a b [c], a [c] b and [c] a b, which is way too many.
Now, if c is another expanding template, that will break into many more other variants which will match, and hassil will check them all.
TL;DR: as a general rule of thumb, don't add completely optional parts to a permutation.
- Your proposed sentence would match stuff like
set of living room lights to 43 percent the brightnessor, worse still,set 43% of the living room lights to light, which doesn't make sense.
All templates you add should NOT match any sentence you don't want to match. That's the reason they were split into 3 templat 8000 es originally.
There was a problem hiding this comment.
Ah, I see what you mean about the permutation issues. I made sure to split up sentences further so optional permutations don't exist as well as tried to prevent sentences like you used in your second bullet point from occurring.
sentences/en/light_HassLightSet.yaml
Outdated
| - "[<numeric_value_set>] <name> brightness to [the] {brightness_level:brightness}" | ||
| - "[<numeric_value_set>] [the] brightness of <name> to [the] {brightness_level:brightness}" | ||
| - "[<numeric_value_set>] <name> [to] [the] {brightness_level:brightness} brightness" | ||
| light_value: "[to] (<brightness>|[the] {brightness_level:brightness})" |
There was a problem hiding this comment.
I'd extract [to] into the encompassing sentence and leave just the light value in this expansion rule, to make it clearer.
sentences/en/light_HassLightSet.yaml
Outdated
| - "[<numeric_value_set>] [the] brightness of <area> to [the] {brightness_level:brightness}" | ||
| - "[<numeric_value_set>] <area> brightness to [the] {brightness_level:brightness}" | ||
| - "[<numeric_value_set>] <area> [to] [the] {brightness_level:brightness} brightness" | ||
| - "[<numeric_value_set>] ([of] [(<all>|<the>)] <location>;<light_value>;[[the] <brightness_synonyms>])" |
There was a problem hiding this comment.
This is getting very hard to read. I'll happily review it after you'll have fixed the split i mentioned earlier and apply the same to these.
sentences/en/light_HassLightSet.yaml
Outdated
| - "[<numeric_value_set>] <area> [to] [the] {brightness_level:brightness} brightness" | ||
| - "[<numeric_value_set>] ([of] [(<all>|<the>)] <location>;<light_value>;[[the] <brightness_synonyms>])" | ||
| expansion_rules: | ||
| location: "([<light>];[<in>] (<area>|<floor>))" |
There was a problem hiding this comment.
There's an <in_area_floor> expansion rule you can use.
There was a problem hiding this comment.
Nice! Swapped out [<in>] (<area>|<floor>) with <in_area_floor>.
sentences/en/light_HassLightSet.yaml
Outdated
| expansion_rules: | ||
| in_here: "[in] here" | ||
| light_value: "[to] (<brightness>|[the] {brightness_level:brightness})" |
There was a problem hiding this comment.
Maybe this should be a global expansion rule, if it keeps repeating?
There was a problem hiding this comment.
Went ahead and moved light_value to the global expansion rule set.
Also, I went ahead and just removed "here" and "in here" from the global skip_words and further updated the templates to still account for those words showing up in sentences.
sentences/en/light_HassLightSet.yaml
Outdated
| - "[<numeric_value_set>] [the] brightness [of] <name> [to] <brightness>" | ||
| - "[<numeric_value_set>] <name> [to] <brightness> [brightness]" | ||
| - "[<numeric_value_set>] ([of] <name>;[to] <light_value>)" | ||
| - "[<numeric_value_set>] ([of] <name>;[to] <light_value>;[the] <brightness_synonyms>)" |
There was a problem hiding this comment.
- Your proposed sentence would match stuff like
set of living room lights to 43 percent the brightnessor, worse still,set 43% of the living room lights to light, which doesn't make sense.
All templates you add should NOT match any sentence you don't want to match. That's the reason they were split into 3 templates originally.
There was a problem hiding this comment.
New push prevents the second command example from occurring.
The idea of permutation stops making sense to me if the first example isn't acceptable. It might also be worth considering simply making all articles and prepositions part of the skip_words list if the goal is to significantly reduce sentence combinations across the board. One upgrade at a time though.
sentences/en/light_HassLightSet.yaml
Outdated
| - "[<numeric_value_set>] [the] brightness to <brightness>" | ||
| - "[<numeric_value_set>] [the] brightness (<in_here>;to <brightness>)" | ||
| - "[<numeric_value_set>] ([of] [(<all>|<the>)] <location>;[to] <light_value>)" | ||
| - "[<numeric_value_set>] ([of] [(<all>|<the>)] <location>;[the] <brightness_synonyms>) [to] <light_value>" |
There was a problem hiding this comment.
This would match set the upstairs light light to 50%. I don't really think that <location> should contain <light> 🤔
There was a problem hiding this comment.
Yeah, this is one of those sentence types that are grammatically correct but sound pretty weird since "light" works both as a way of defining objects that produce luminescence and a unit of measurement for luminescence; similar to saying something like the orange orange. Sure, set the upstairs light light sounds weird; but, set the upstairs upstairs lamp light still makes sense to say despite probably fitting better in the Victorian era.
While there are some ways to clean this up, I don't think removing <light> from <location> is a good idea. It's the whole reason I created this pull request in the first place since there wasn't a way before to say a command like set the lights to 50% which we can do when strictly turning lights on and off.
Personally I'd like to leave this as is; but if that would prevent this from getting accepted, what I'd recommend doing instead is removing "light" as a brightness synonym instead and removing any test sentences that depended on that. Something like light light couldn't happen anymore but we still would maintain backwards compatibility with the voice commands for turning lights on and off while only losing sentences that are less likely to be missed such as set bedroom to 50% light.
…d optimized local expansion_rule.
sentences/en/light_HassLightSet.yaml
Outdated
| - "[<numeric_value_set>] [<the>] <light> ([to] <brightness_value>;<here>)" | ||
| - "[<numeric_value_set>] [(<all>|<the>)] <light> ([to] <brightness_value>;<here>)" |
There was a problem hiding this comment.
These can be contracted, right?
| - "[<numeric_value_set>] [<the>] <light> ([to] <brightness_value>;<here>)" | |
| - "[<numeric_value_set>] [(<all>|<the>)] <light> ([to] <brightness_value>;<here>)" | |
| - "[<numeric_value_set>] [(<all>|<the>)] <light> ([to] <brightness_value>;<here>)" |
There was a problem hiding this comment.
Yep, another good catch. I went ahead and made this change as well as the similar suggestion below.
sentences/en/light_HassLightSet.yaml
Outdated
| - "[<numeric_value_set>] [<the>] <light> (([to] <brightness_value>;<brightness_synonyms>);<here>)" | ||
| - "[<numeric_value_set>] [(<all>|<the>)] <light> (([to] <brightness_value>;<brightness_synonyms>);<here>)" |
There was a problem hiding this comment.
| - "[<numeric_value_set>] [<the>] <light> (([to] <brightness_value>;<brightness_synonyms>);<here>)" | |
| - "[<numeric_value_set>] [(<all>|<the>)] <light> (([to] <brightness_value>;<brightness_synonyms>);<here>)" | |
| - "[<numeric_value_set>] [(<all>|<the>)] <light> (([to] <brightness_value>;<brightness_synonyms>);<here>)" |
sentences/en/light_HassLightSet.yaml
Outdated
| expansion_rules: | ||
| in_here: "[in] here" | ||
| - "[<numeric_value_set>] ([to] <brightness_value>;[the] <brightness_synonyms>)" | ||
| - "[<numeric_value_set>] ([to] <brightness_value>;[the] <brightness_synonyms>;<here>)" |
There was a problem hiding this comment.
This is too permissive, I think
set to 50% here the brightness
set here to 50% the brightness
Maybe this is better?
| - "[<numeric_value_set>] ([to] <brightness_value>;[the] <brightness_synonyms>;<here>)" | |
| - "[<numeric_value_set>] ([to] <brightness_value>;[the] <brightness_synonyms> [<here>])" |
There was a problem hiding this comment.
Makes sense to me. Went ahead and made this suggested change as well and updated a few of the tests accordingly.
Extends the list of sentences that can trigger light_HassLightSet intent to include the brightness synonyms "light" and "lights".
all uses of the word "brightness" are now mapped to the expansion rule brightness_synonyms. Results in some minor odd sounding acceptable sentences such as "set the lights of the bedroom to 50% lights" but with the added benefit of the following sentences:
"set the lights in the bedroom to 50%"
"set the bedroom lights to the max"
"set the bedroom lamp to minimum light"
Change to expansion rule also allows for trivial addition of other brightness synonyms such as "illumination" or "luminosity" for those with a finer word palette. Anything unique enough for the system to infer that the intent must be light based.
Added tests duplicate brightness cases but with intermittent changes with either the word "light" or "lights" to avoid a complete duplication of tests for each synonym.