davidusb-geek / emhass

emhass: Energy Management for Home Assistant, is a Python module designed to optimize your home energy interfacing with Home Assistant.
MIT License
260 stars 51 forks source link

Implement start penalty functionality #291

Closed werdnum closed 1 month ago

werdnum commented 1 month ago

How to use:

I also made a couple of other supporting changes:

werdnum commented 1 month ago

Solves #261

davidusb-geek commented 1 month ago

Hi @werdnum, is this ready to merge? Probably some documentation needed somewhere. From what I see this is only usable from the runtime parameters dictionary, but not from the configuration file?

werdnum commented 1 month ago

I think it works in the configuration file. I’ll have to check to make sure.

Will take a look at documentation.

— Andrew Garrett

On Tue, 28 May 2024 at 23:11, David @.***> wrote:

Hi @werdnum https://github.com/werdnum, is this ready to merge? Probably some documentation needed somewhere. From what I see this is only usable from the runtime parameters dictionary, but not from the configuration file?

— Reply to this email directly, view it on GitHub https://github.com/davidusb-geek/emhass/pull/291#issuecomment-2135183717, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACCFXV7AF5J4772D6EEAJDZER7BFAVCNFSM6AAAAABIEWH3ROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZVGE4DGNZRG4 . You are receiving this because you were mentioned.Message ID: @.***>

davidusb-geek commented 1 month ago

Ok, I'm merging. But from what I see this will only works when passing def_start_penalty and def_current_state at runtime. For def_current_state it makes completely sense, but for def_start_penalty this could be set directly in the configuration file. A small entry on the documentation would be needed don't you think. I will merge and then try to update the documentation later on another PR

davidusb-geek commented 3 weeks ago

Hi, this was merged but not correctly reviewed I guess. I was trying to update the documentation an stumbled on this. There are some important things that I don't fully understand:

Let's start with this code for def_start_penalty:

        # Add term penalizing each startup where configured
        if ("def_start_penalty" in self.optim_conf and self.optim_conf["def_start_penalty"]):
            for k in range(self.optim_conf["num_def_loads"]):
                if (len(self.optim_conf["def_start_penalty"]) > k and self.optim_conf["def_start_penalty"][k]):
                    objective = objective + plp.lpSum(
                        -0.001 * self.timeStep * self.optim_conf["def_start_penalty"][k] * P_def_start[k][i] *\
                            unit_load_cost[i] * self.optim_conf['P_deferrable_nom'][k]
                        for i in set_I)

This self.optim_conf["def_start_penalty"] is supposed to be a list of floats. But it seems to me that you are treating this a list of bool. For example here if ("def_start_penalty" in self.optim_conf and self.optim_conf["def_start_penalty"]). Another term I don't understand is len(self.optim_conf["def_start_penalty"]) > k. If that parameter is a list of floats with a single float for each deferrable then when is this condition supposed to True? And again this is treating self.optim_conf["def_start_penalty"][k] as if it were a bool.

Same comment for the def_current_state parameter. This condition can never hold if that parameter is a list of bool with a single bool for each deferrable load: if ("def_current_state" in self.optim_conf and len(self.optim_conf["def_current_state"]) > k): I'm a bit lost here.

werdnum commented 3 weeks ago

Hi David, thanks for taking a look and sorry I haven't got to the documentation yet!

Happy to help you understand the code and of course if there's something I've done that's hard to understand, please let me know how I can make it better.

I am relatively confident in the basic functionality because of the expanded unit tests, but there could be small bugs of course.

— Andrew Garrett

On Mon, 10 Jun 2024 at 05:34, David @.***> wrote:

Hi, this was merged but not correctly reviewed I guess. I was trying to update the documentation an stumbled on this. There are some important things that I don't fully understand:

Let's start with this code for def_start_penalty:

    # Add term penalizing each startup where configured
    if ("def_start_penalty" in self.optim_conf and self.optim_conf["def_start_penalty"]):
        for k in range(self.optim_conf["num_def_loads"]):
            if (len(self.optim_conf["def_start_penalty"]) > k and self.optim_conf["def_start_penalty"][k]):
                objective = objective + plp.lpSum(
                    -0.001 * self.timeStep * self.optim_conf["def_start_penalty"][k] * P_def_start[k][i] *\
                        unit_load_cost[i] * self.optim_conf['P_deferrable_nom'][k]
                    for i in set_I)

This self.optim_conf["def_start_penalty"] is supposed to be a list of floats. But it seems to me that you are treating this a list of bool. For example here if ("def_start_penalty" in self.optim_conf and self.optim_conf["def_start_penalty"]).

This is a common idiom in python - a nonzero value evaluates to true, a zero value (or empty string, None, etc) evaluates to false. I'm checking whether there is any value (so I don't just assume it's a list and crash if there's nothing there). This is what's recommended by the Google style guide ( https://google.github.io/styleguide/pyguide.html) but happy to follow your lead if you have another style preference.

Another term I don't understand is len(self.optim_conf["def_start_penalty"])

k. If that parameter is a list of floats with a single float for each deferrable then when is this condition supposed to True?

Here I'm trying to check whether the list of start penalties is long enough to have a value for the current one. I don't want to try to get a staff penalty value for load 2 (3rd in the list) if there are only 2 items in the list. Len() returns the number of values, and k is the zero based index of the deferrable load. So if there are 2 deferrable loads, K could be 0 or 1.

And again this is treating self.optim_conf["def_start_penalty"][k] as if it were a bool.

Right, I don't want to do any processing for start penalty if it's set to zero.

Same comment for the def_current_state parameter. This condition can never hold if that parameter is a list of bool with a single bool for each deferrable load: if ("def_current_state" in self.optim_conf and len(self.optim_conf["def_current_state"]) > k):

Remember K will take all values from 0 to num_deferrable_loads exclusive, so this will be false only when the def_current_state list if under sized.

I'm a bit lost here.

— Reply to this email directly, view it on GitHub https://github.com/davidusb-geek/emhass/pull/291#issuecomment-2156752354, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACCFXTTMMWZHAP6RPLSWSTZGSU2XAVCNFSM6AAAAABIEWH3ROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWG42TEMZVGQ . You are receiving this because you were mentioned.Message ID: @.***>