feature: interpret load sensors in kW rather than W

[pjvenda]

I took a look and tried to implement a switch that has load sensors interpreted as kW rather than W. I ran the tests and the optimisation process and I think it is working. Anything else I should be testing for? Thanks in advance.

[davidusb-geek]

Hi, thanks for working on this.
Two quick comments.
The name of the new control variable is load_sensor_kw but it applies to both the load power and the PV power. That is confusing and not generalized, for example to cover the case when the load power is in kW but the PV power is not.
My second comment is just about the tests, you adapted the tests to this change and maybe I missed something but I don't see where you actually test this. Provide just a minimum and quick test in test_retrieve_hass.py where you actually set the load_sensor_kw=True and you then check numerically that the factor is applied.

[davidusb-geek]

The name of the new control variable is load_sensor_kw but it applies to both the load power and the PV power. That is confusing and not generalized, for example to cover the case when the load power is in kW but the PV power is not.

So to solve this probably break it into two new variables, something like: var_PV_in_kW and var_load_in_kW should do I guess. What do you think?

[pjvenda]

Thanks for reviewing!

Your comments make sense.
I'll split the control variables and use your naming scheme.
I may need help to implement the test correctly though. I'll try.

[pjvenda]

The name of the new control variable is load_sensor_kw but it applies to both the load power and the PV power. That is confusing and not generalized, for example to cover the case when the load power is in kW but the PV power is not.

So to solve this probably break it into two new variables, something like: var_PV_in_kW and var_load_in_kW should do I guess. What do you think?

Please check how this version looks like to you. I've split the unit flags and I've added the same option to the methods that take the sensors as runtime parameters. The concept is that these apply only when collecting info from HA, cached data should be kept in W, so methods that read off files shouldn't be checking the flags.

[pjvenda]

identical situation to other runtimeparams including other boolean flags

[davidusb-geek]

Unit tests are failing. This doesn't seem ready yet?

[pjvenda]

Hello David, apologies I am not used to working with GitHub or pull request etiquette. Should I undo the pull request until it is clean and ready to go? Is there a way to run the unit tests outside of this interface? Thanks, Pedro.

…

On Sat, 1 Jun 2024, 18:10 David, ***@***.***> wrote: Unit tests are failing. This doesn't seem ready yet? — Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APVEFGQF6QHMQH7ZV6QBGP3ZFH6ANAVCNFSM6AAAAABIHZXT46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTGUYTOMZYGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[davidusb-geek]

Hello David, apologies I am not used to working with GitHub or pull request etiquette. Should I undo the pull request until it is clean and ready to go? Is there a way to run the unit tests outside of this interface? Thanks, Pedro.
…
On Sat, 1 Jun 2024, 18:10 David, @.> wrote: Unit tests are failing. This doesn't seem ready yet? — Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/APVEFGQF6QHMQH7ZV6QBGP3ZFH6ANAVCNFSM6AAAAABIHZXT46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTGUYTOMZYGY . You are receiving this because you authored the thread.Message ID: @.>

No problem.
I'm just releasing a new version and I was checking if this was ready.
Yes you can totally test it in many ways. Complete guide here:
https://emhass.readthedocs.io/en/latest/develop.html

For me Method 1 locally works just fine.
But the containers are also a very good option.

[Octofinger]

@pjvenda Sorry to be barking in this late in your development process, but I'd like to share my thoughts on the configuration variable.
I believe using a boolean variable to select whether to use kW units or not is a bad idea. It's a bad idea because it doesn't explicitly say what unit is used if you set the variable to False. It would be much more clear from an end user perspective if you use the configuration variable to actually define the unit. E.g. something like this:

var_PV_power_unit that can take the values "kW" or "W"
var_load_power_unit that can take the values "kW" or "W"

So in config, you define the unit as var_PV_power_unit: 'W'.
In the code, lower case the configured value (so it's not case sensitive in the config) and also set a default value of 'W' in case the config variable isn't set.

[pjvenda]

No need to apologise. I have been stuck with work and fam life with little chance to advance the development. I take your point, it makes sense. On the code side however, taking a text parameter is much more tricky to parse than a true/false as there is more variability and more unknowns, so at some point a Boolean would exist internally regardless. Another option is to deal with this at the UI level (e.g. radio box for W or kW) and just hide the Boolean altogether.

…

On Wed, 30 Oct 2024, 07:58 Octofinger, ***@***.***> wrote: @pjvenda <https://github.com/pjvenda> Sorry to be barking in this late in your development process, but I'd like to share my thoughts on the configuration variable. I believe using a boolean variable to select whether to use kW units or not is a bad idea. It's a bad idea because it doesn't explicitly say what unit is used if you set the variable to False. It would be much more clear from an end user perspective if you use the configuration variable to actually define the unit. E.g. something like this: var_PV_power_unit that can take the values "kW" or "W" var_load_power_unit that can take the values "kW" or "W" So in config, you define the unit as var_PV_power_unit: 'W'. In the code, lower case the configured value (so it's not case sensitive in the config) and also set a default value of 'W' in case the config variable isn't set. — Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APVEFGUJRYZXYCXTAUG4AH3Z6CGTDAVCNFSM6AAAAABIHZXT46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBWGEYDENJQGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Octofinger]

Thank you. Your code, your decision.

Just remember that there's no UI option for the stand-alone setups, so configuration must be reasonably understandable in yaml.
We already have a number of text options, number options and array options in the configuration, perhaps you can reuse some of the input validation logic from those? It doesn't really matter if the option will be used as a boolean internally, as this is hidden from the end user, but it would make a difference in making the configuration more self-explanatory, something that EMHASS is lacking today. Implementing something that's easier to code but harder to use would only build additional technical debt.

I think it would pay off in the end to make this a string configuration to set the actual power unit to use, even if it would translate to a boolean value internally.

Thanks for contributing to EMHASS though. I wish I could find the time and skills to do more myself.
//O

No need to apologise. I have been stuck with work and fam life with little chance to advance the development. I take your point, it makes sense. On the code side however, taking a text parameter is much more tricky to parse than a true/false as there is more variability and more unknowns, so at some point a Boolean would exist internally regardless. Another option is to deal with this at the UI level (e.g. radio box for W or kW) and just hide the Boolean altogether.
…
On Wed, 30 Oct 2024, 07:58 Octofinger, @.> wrote: @pjvenda https://github.com/pjvenda Sorry to be barking in this late in your development process, but I'd like to share my thoughts on the configuration variable. I believe using a boolean variable to select whether to use kW units or not is a bad idea. It's a bad idea because it doesn't explicitly say what unit is used if you set the variable to False. It would be much more clear from an end user perspective if you use the configuration variable to actually define the unit. E.g. something like this: var_PV_power_unit that can take the values "kW" or "W" var_load_power_unit that can take the values "kW" or "W" So in config, you define the unit as var_PV_power_unit: 'W'. In the code, lower case the configured value (so it's not case sensitive in the config) and also set a default value of 'W' in case the config variable isn't set. — Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/APVEFGUJRYZXYCXTAUG4AH3Z6CGTDAVCNFSM6AAAAABIHZXT46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBWGEYDENJQGE . You are receiving this because you were mentioned.Message ID: @.>