Open pjvenda opened 1 month ago
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.
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?
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.
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
andvar_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.
Unit tests are failing. This doesn't seem ready yet?
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 https://github.com/davidusb-geek/emhass/pull/292#issuecomment-2143517386, or unsubscribe https://github.com/notifications/unsubscribe-auth/APVEFGQF6QHMQH7ZV6QBGP3ZFH6ANAVCNFSM6AAAAABIHZXT46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTGUYTOMZYGY . You are receiving this because you authored the thread.Message ID: @.***>
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.
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.