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

feature: interpret load sensors in kW rather than W #292

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pjvenda
Copy link

@pjvenda pjvenda commented May 24, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

pjvenda commented May 24, 2024

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.

…ly. modified methods that take runtime parameters to accept a var_model_in_kw boolean for the same purpose.
@pjvenda
Copy link
Author

pjvenda commented May 28, 2024

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.

if "var_model_in_kw" not in runtimeparams.keys():
var_model_in_kw = False
else:
var_model_in_kw = eval(str(runtimeparams["var_model_in_kw"]).capitalize())

Check failure

Code scanning / CodeQL

Code injection Critical

This code execution depends on a
user-provided value
.
This code execution depends on a
user-provided value
.
Copy link
Author

Choose a reason for hiding this comment

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

identical situation to other runtimeparams including other boolean flags

@davidusb-geek
Copy link
Owner

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

@pjvenda
Copy link
Author

pjvenda commented Jun 1, 2024 via email

@davidusb-geek
Copy link
Owner

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.

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