Skip to content

[en] Refactor HassLightSet using LLM TDD and new expansions #2783

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ViViDboarder
Copy link
Contributor

This should make the matching more robust and much easier to manage.

This depends on #2728 getting merged first.

This should make the matching more robust and much easier to manage.
@ViViDboarder ViViDboarder marked this pull request as ready for review January 8, 2025 14:44
@ViViDboarder ViViDboarder requested a review from tetele January 8, 2025 14:45
out: 100
- in: (min|minimum|lowest)
- in: "[the] (half|halfway|mid) [(level|setting|brightness|bright|light|lit)]"
Copy link
Contributor

Choose a reason for hiding this comment

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

"The mid lit" doesn't sound right

Suggested change
- in: "[the] (half|halfway|mid) [(level|setting|brightness|bright|light|lit)]"
- in: "([the] (half|halfway|mid) [(level|setting|brightness|light)]|(half|halfway|mid) (bright|lit)"

- "[<numeric_value_set>] [[(<all>|<the>)] <light>] [brightness] [to] <brightness> <in_area_floor>"
- "[<numeric_value_set>] [(<all>|<the>)] <light> <in_area_floor> [brightness] [to] <brightness>"

- "[<numeric_value_set>] [the] brightness [of [(<all>|<the>)]] <in_area_floor> [<light>] [to] <brightness>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- "[<numeric_value_set>] [the] brightness [of [(<all>|<the>)]] <in_area_floor> [<light>] [to] <brightness>"
- "[<numeric_value_set>] [the] brightness [of [(<all>|<the>)]] <area_floor> [<light>] [to] <brightness>"

- "[<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>] [(<all>|<the>)] <area_floor> [<light>] [brightness] [to] <brightness>"
Copy link
Contributor

Choose a reason for hiding this comment

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

This would match "living room 50%" which, again, may not necessarily mean brightness. It could mean fan speed, it could mean many more things. It's too ambiguous

Comment on lines +36 to +38
- "[<brightness_relative>] [(<all>|<the>)] <area_floor> [<light>] [to] <brightness>"
- "[<brightness_relative>] [[(<all>|<the>)] <light>] [to] <brightness> <in_area_floor>"
- "[<brightness_relative>] [(<all>|<the>)] <light> <in_area_floor> [to] <brightness>"
Copy link
Contributor

Choose a reason for hiding this comment

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

With optional <brightness_relative>, these are the same sentences as above (with <numeric_value_set>. Remove the optional part.

- "[<numeric_value_set>] [(<all>|<the>)] <light> [brightness] [<here>] [to] <brightness>"
- "[<numeric_value_set>] [(<all>|<the>)] <light> [brightness] [to] <brightness> <here>"

- "[<brightness_relative>] [[(<all>|<the>)] <light>] [<here>] [to] <brightness>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch! "Hey Nabu... 70%" 😅 Definitely no!

- "[<numeric_value_set>] [(<all>|<the>)] <light> [brightness] [to] <brightness> <here>"

- "[<brightness_relative>] [[(<all>|<the>)] <light>] [<here>] [to] <brightness>"
- "[<brightness_relative>] [[(<all>|<the>)] <light>] [to] <brightness> <here>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing, "70% here" is too vague.

@home-assistant home-assistant bot marked this pull request as draft January 8, 2025 15:12
@home-assistant
Copy link

home-assistant bot commented Jan 8, 2025

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@ViViDboarder
Copy link
Contributor Author

🤦‍♂️ I got a little carried away it seems. I need to split these to multiple sentences to make sure some of the optional are mutually exclusive.

@ViViDboarder
Copy link
Contributor Author

ViViDboarder commented Jan 16, 2025

I've been trying to think of a good way to address this feedback and one of the things I'm coming to is possibly to break apart something like <brightness_value> and <brightness>. By that I mean, capturing the level vs capturing the words that indicate a brightness intent.

Because right now something like <area> [<lights>] to {brightness} could match kitchen to 80%, when I really want to match either kitchen lights to 80% or kitchen to 80% bright. I'm imagining this then being represented by two sentences <area> <lights> to <brightness_value> [<brightness>] and <area> [<lights>] to <brightness_value> <brightness>

This does mean some possible less used sentences will creep in because there won't be mutual exclusivity between {brightness_level} and <brightness> intent indicators. So it would introduce matches for kitchen to mid lit and other unlikely sentences that are technically unambiguous, but not helpful. This could be remedied by splitting both <brightness_value> and <brightness> into two each that have some distinction (not sure what I'd call them...) and then doubling the base sentences, but that all sounds like a lot more complexity to avoid a couple unuseful sentences.

I'll make the refactor and we can debate another further split after.

@ViViDboarder
Copy link
Contributor Author

Oh! I just thought of an alternative that could avoid the vague matches like room 50%. what about required keywords? If a list of required keywords included <light> + (bright|brightness|light|lit|brighten|lighten|dark|darken|dim), then all the ambiguous sentences would be skipped.

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

Successfully merging this pull request may close these issues.

[EN] Can't set brightness of lights in a zone without specifying the light name
2 participants