feat(scheduling): Price devices distinctively in scheduling #654
Conversation
flexmeasures/cli/data_add.py
Outdated
| @@ -1103,6 +1119,8 @@ def add_schedule_for_storage( | |||
| "consumption-price-sensor": consumption_price_sensor.id, | |||
| "production-price-sensor": production_price_sensor.id, | |||
| "inflexible-device-sensors": [s.id for s in inflexible_device_sensors], | |||
| "consumption-price-sensors-per-device": {(power.id, price.id) for (power, price) in consumption_price_sensors_per_device.items()}, | |||
| "production-price-sensors-per-device": {(power.id, price.id) for (power, price) in production_price_sensors_per_device.items()}, | |||
There was a problem hiding this comment.
As the schema expects a dict, wouldn't it be better to pass the dicts here as-is?
There was a problem hiding this comment.
@Srieon @rajath-09, this is something we could do ...
There was a problem hiding this comment.
As the schema expects a dict, wouldn't it be better to pass the dicts here as-is?
Addressed.
flexmeasures/cli/data_add.py
Outdated
| "consumption_price_sensor_per_device", | ||
| type=dict, | ||
| required=False, | ||
| help="", |
There was a problem hiding this comment.
Please specify help on what this command option is about. Probably text from our earlier discussions or the issue can be used to start.
There was a problem hiding this comment.
@nhoening, yes, i've added that to TODO.
|
And the PR title could be more informative, as well :) |
anirudh-ramesh
changed the title
There was a problem hiding this comment.
Hi @rajath-09 - happy to help over here 😄.
I can't add comments on code that hasn't been edited in this PR so I'll try to explain myself in this comment and some other comments in the code review.
I think you need to add a new index dimension, the device, to:
-
the variables
commitment_downwards_deviationandcommitment_upwards_deviation -
the parameters
up_priceanddown_price
For instance, for the up_price:
model.up_price = Param(model.c, model.j, model.d, initialize=price_up_select)Moreover, you need to add the new dimension d to the initialization functions:
price_down_selectprice_up_select
Finally, you need to add the new dimension d to the cost_function as I noted.
Please, if you could provide us with some logs and/or steps to reproduce the error it would help a lot.
I hope this helpful.
Thanks
anirudh-ramesh
changed the title
|
Hi @victorgarcia98 . I have made the changes you suggested in the latest PR. I have added the new index dimension 'p' ,denoting the set of price sensors ,to the variables and parameters. |
|
Hi @rajath-09,
Isn't that each device has a different price? The array of prices might repeat the same price for multiple devices that share it.
I don't see anything here. Could you please share with us the code + data that you are using to test this? We can do it privately if you wish to. Cheers, |
|
Cool, I will connect with you privately. |
|
Also, I see that model.up_price = Param(model.c, model.j, initialize=price_up_select)
model.down_price = Param(model.c, model.j, initialize=price_down_select)require the parameter |
I guess i might have missed on that in the PR,but the "infeasible model" issue pertains. |
|
This PR needs an automated test anyway, and I suggest that this is a good time to make that happen. We need a shared coded definition what success is. Otherwise we cannot really help you without a lot of added effort. You can find tests we wrote to test scheduling in this repo. Copy one and adjust to your case. |
Adding index dimension to model.up_price and model.down_price Signed-off-by: rajath-09 <85490216+rajath-09@users.noreply.github.com>
Updating ems_flow_commitment_equalities function Signed-off-by: rajath-09 <85490216+rajath-09@users.noreply.github.com>
Adding another index dimension. Signed-off-by: rajath-09 <85490216+rajath-09@users.noreply.github.com>
|
Hey @victorgarcia98 |
|
Hi @rajath-09 and @anirudh-ramesh, as I wrote last week, this PR needs an automated test. Please don't send my team assets and data from your projects to test locally as a habit. It was useful as a one-time approach, so we could help you solve a problem. But in the end, we will not accept this PR without a reproducible test case. |
|
Hi @nhoening , the data and assets were sent just in case the team needed to test out the new commits. Meanwhile,we understand the need for an automated test in PR and we are working on it and will get back to you with that when we are done. |
|
Great, looking forward! The test would have been already useful in demonstrating the error. |
If I am not wrong I have reacted to all of them except for the 'prepare_data' feature which we believe could be part of another PR as of now.Alternatively checking on the 'planned_costs' will give a clear picture. |
|
Superglad to hear that this is already tested functionality-wise on your end! Please hit all "Resolve conversation" buttons that are not relevant anymore. We should decide who is the best reviewer to go further towards merging. @victorgarcia98, are you into this PR enough? Feel free to converse in our chat for support. |
Sure! I can review it. |
flexmeasures/cli/data_add.py
Outdated
| "consumption_price_sensor_per_device", | ||
| type=dict, | ||
| required=False, | ||
| help="", |
|
I have already resolved all conversations but it still shows 2 conversation unresolved.Not sure why though? |
This has happened to me before. It might be related to that I'm doing a review. |
Ok let me know if something has to be done at my end. |
| "production_price_sensor_per_device", | ||
| type=dict, | ||
| required=False, | ||
| help="Optimize production against this dictionary of sensors. Defaults to the consumption price sensor. The sensors typically record electricity prices (e.g. in EUR/kWh), but this field can also be used to optimize against some emission intensity factors (e.g. in kg CO₂ eq./kWh).", |
There was a problem hiding this comment.
Here I would show an example of how to pass a dictionary in the command line.
Moreover, I'm not sure this type (dict) would work straightaway, given that:
- The dict type is not listed in the click parameters.
- I created a simple command to test it and I couldn't get to accept any of the formats
{"a" : 2},a=1,"a"=1,a:1or'{"a":2}'.
Test command:
@fm_add_data.command("test-dict")
@click.option("--dictionary", type=dict)
def test_dict_type(dictionary : dict):
click.secho(dictionary)There was a problem hiding this comment.
Correctly said.I propose we do something like this?
@click.option("--dictionary", type=str)
def test_dict_type(dictionary: str):
try:
dictionary = json.loads(dictionary)
click.secho(str(dictionary))
except json.JSONDecodeError:
click.secho("Invalid dictionary format.", fg='red')
Using this now I can use the format { "19" : 21}
Let me know if that works?
There was a problem hiding this comment.
Hey @victorgarcia98
I worked on this now it supports for the following type of input :
flexmeasures add schedule for-storage --sensor-id 1 --consumption-price-sensor-per-device '{"1": 2}' --start ${TOMORROW}T07:00+01:00 --duration PT12H --soc-at-start 50% --roundtrip-efficiency 90% --as-job
Let me know if that's fine?
There was a problem hiding this comment.
Also given the introduction to the new price sensor parameters, I believe the consumption_price_sensor should no longer be a necessary argument in the CLI and rather an option.It worked for me by omitting the function call to replace_deprecated_argument.Is that how it is supposed to be done?
| allow_trimmed_query_window=False, | ||
| ) | ||
| up_deviation_prices_array = [] | ||
| for power_sensor, price_sensor in consumption_price_sensor_per_device.items(): |
There was a problem hiding this comment.
What if consumption_price_sensor is provided instead of consumption_price_sensor_per_device?
There was a problem hiding this comment.
For example, if the StorageScheduler is called through the api, it will fail.
There was a problem hiding this comment.
Agreed. Shall we just use if case to handle that ?
There was a problem hiding this comment.
Alternatively, I was wondering if I could append the 'consumption_price_sensor' to 'consumption_price_sensor_per_device' ,and similarly for production, by making the dictionary link between 'sensor=self.sensor' with 'consumption_price_sensor'?
What do you think about that?If you agree I will proceed with that.
There was a problem hiding this comment.
Yes, I think that could work. If consumption_price_sensor_per_device is not provided, you can create internally one that maps each device to the same consumption_price_sensor. The same for production.
There was a problem hiding this comment.
Do I need to map each device with consumption_price_sensor or could i just map consumption_price_sensor with the battery sensor either?Shouldn't that work as well?
There was a problem hiding this comment.
If consumption_price_sensor_per_device is not provided, you can create internally one that maps each device to the same consumption_price_sensor. The same for production.
This would not be the same as applying the consumption price sensor to the aggregate power flow, which is the use case we currently support.
For example, for a given time step, we currently have:
Example A
- Device A consumes 10 kWh
- Device B produces 8 kWh
- Devices A and B belong to the same system, with an aggregate consumption of 2 kWh
- The system's aggregate consumption is priced at 0.5 EUR/kWh
- The system costs are 1 EUR.
If I am reading your suggestion right, you propose:
Example B
- Device A consumes 10 kWh, priced at 0.5 EUR/kWh, incurring costs of 5 EUR
- Device B produces 8 kWh, priced at 0.5 EUR/kWh, incurring a revenue of 4 EUR
- The system costs are 5 - 4 = 1 EUR
Is that what you had in mind?
This might look the same, but things change when the consumption price differs from the production price. Let's say the production price is 0.4 EUR/kWh. Example A wouldn't change, because there is no aggregate production, only aggregate consumption. Example B changes, however:
- Device A consumes 10 kWh, priced at 0.5 EUR/kWh, incurring costs of 5 EUR
- Device B produces 8 kWh, priced at 0.4 EUR/kWh, incurring a revenue of 3.20 EUR
- The system costs are 5 - 3.20 = 1.80 EUR
In my opinion, we should not touch the current use case by trying to map the consumption_price_sensor argument to individual devices. Instead, we should extend the cost function with additional cost components for each device, allowing to specify a consumption and production price per device. If a price for a specific device is not specified, it can be assumed to be zero.
There was a problem hiding this comment.
Yes agreed @victorgarcia98 .But what I was suggesting was to map consumption_price_sensor to the battery sensor(which would in turn take care of the aggregate consumption/production just like in Example A). In my opinion ,that's gonna work perfectly as mapping it with the battery would mean mapping it up with the system.Let me know if that's something that works?
There was a problem hiding this comment.
I don't follow. Can you explain it with an example in more detail, with example values?
There was a problem hiding this comment.
Earlier, we had the following:
up_deviation_prices, (start, end) = get_prices(
(start, end),
resolution,
beliefs_before=belief_time,
price_sensor=consumption_price_sensor,
sensor=sensor,
allow_trimmed_query_window=False,
)
which now has changed to this:
up_deviation_prices_array = []
for power_sensor, price_sensor in consumption_price_sensor_per_device.items():
up_deviation_prices, (start, end) = get_prices(
(start, end),
resolution,
beliefs_before=belief_time,
price_sensor=price_sensor,
sensor=power_sensor,
allow_trimmed_query_window=False,
)
up_deviation_prices_array.append(up_deviation_prices)
Keeping this in mind, I believe the sensor to be passed to get_prices should be the battery power sensor to make sure the price sensor is applied to the aggregate power flow.
So the following code should be added to compute function :
#Convert single price sensors to Multiple Price Sensors Dict
if consumption_price_sensor is not None:
consumption_price_sensor_per_device[sensor]=consumption_price_sensor
if production_price_sensor is not None:
production_price_sensor_per_device[sensor]=production_price_sensor
This will make the situation exactly like Example A.There is no difference in the example.
| @@ -29,8 +30,12 @@ def device_scheduler( # noqa C901 | |||
| device_constraints: List[pd.DataFrame], | |||
| ems_constraints: pd.DataFrame, | |||
| commitment_quantities: List[pd.Series], | |||
| commitment_downwards_deviation_price: Union[List[pd.Series], List[float]], | |||
| commitment_upwards_deviation_price: Union[List[pd.Series], List[float]], | |||
| consumption_price_sensor_per_device: Dict[int, int], | |||
There was a problem hiding this comment.
Not sure if device_scheduler function is used somewhere else (e.g a plugin). In any case, I think we should still support the previous way (i.e commitment_downwards_deviation_price, commitment_upwards_deviation_price). What do you think @Flix6x?
There was a problem hiding this comment.
Indeed we should make this backwards compatible instead of introducing a change that potentially breaks things.
At the moment, I think this constitutes a breaking change not only in terms of changing the function signature, but also in terms of breaking a use case. Namely, applying prices to the whole system rather than to each device individually.
| commitment_downwards_deviation_price_array: List[ | ||
| Union[List[pd.Series], List[float]] | ||
| ], | ||
| commitment_upwards_deviation_price_array: List[Union[List[pd.Series], List[float]]], | ||
| ) -> Tuple[List[pd.Series], float, SolverResults]: | ||
| """This generic device scheduler is able to handle an EMS with multiple devices, | ||
| with various types of constraints on the EMS level and on the device level, |
There was a problem hiding this comment.
The docstring is not updated with the new parameters.
| @@ -191,8 +210,8 @@ def device_derivative_up_efficiency(m, d, j): | |||
| return 1 | |||
| return eff | |||
|
|
|||
| model.up_price = Param(model.c, model.j, initialize=price_up_select) | |||
| model.down_price = Param(model.c, model.j, initialize=price_down_select) | |||
| model.up_price = Param(model.u, model.c, model.j, initialize=price_up_select) | |||
There was a problem hiding this comment.
I think that instead of using u and p indexes, the price should depend only on the device (d).
There was a problem hiding this comment.
Using 'u','p' allows the user to have different number or 'consumption_price_sensor_per device' and 'production_price_sensor_per_device'. Using 'd' would not serve that purpose.
There was a problem hiding this comment.
Correct me if I'm wrong, the idea of this PR is to allow having different price sensor between devices. That means that we want to map each device to a price sensor.
For example:
Device 1 -> Consumption 1, Production 2
Device 2 -> Consumption 3, Production 4
Device 3 -> Consumption 5, Production 6
In this example, we would have consumption_price_sensor_per_device = [1,3,5] and production_price_sensor_per_device = [2,4,6].
Having the `consumption_price_sensor_per_device[d]' indexed by the device, we could get the corresponding price sensor to each device. This is, of course, assuming that we can have only 2 price sensors per device (consumption and production).
Another interpretation of having 'allow having different price sensor between devices could be to allow defining multiple price sensors to a device. In that case, there are different ways to have multiple price sensors "attached" to a device: sum, different times of the day. Is this later interpretation the one that you had in mind?
There was a problem hiding this comment.
My interpretation was on the basis of the assumption that a device may have only one price sensor attached to it.For example,
- Solar Plants will have only Production Price Sensor as they ideally wont be consuming electricity
- Load/Building will have consumption price sensor only as they are supposed to only consume electricity.
Let me know if my understanding is right.
There was a problem hiding this comment.
Thanks, we are on the same page!
Then, we can use the device index d (instead of u and p) to get which price time series correspond to a particular device, given that there would be at most one.
Given a dictionary that maps devices to the prices, we can iterate through all the devices and in case the price is missing in the dictionary, we just have one with price zero, as @Flix6x mentioned in another conversation.
There was a problem hiding this comment.
Sure @victorgarcia98 .That works.
But couple of things i needed to clarify before working it through:
- Is there any particular problem with the introduction of the new index dimensions?
- For instance,grid is a flexible device.So in that case ,I wont be able to map price sensors to the grid because 'd' here refers to the set of inflexible devices.How will that be taken care of?
- Will putting price zero to some price sensor mean that the device can consume/produce at cost 0 or will it restrict the consumption/production of energy for that device(in case consumption/production is set to zero)?According to my understanding it will be the first case and that will make our model give different results.
Please let me know if you understood my concerns.Also let me know that you think?
There was a problem hiding this comment.
-
Applying prices to the power flow exchanged with the grid is exactly the use case we currently support (and want to keep supporting). We do not explicitly model the grid as a separate device, but rather model it implicitly as the sum of (the power flow of) all devices. Grid (power) constraints can be set through the variable
ems_constraints. Grid prices can be set separately for consumption and production, usingconsumption_price_sensorandproduction_price_sensor, respectively. -
A zero device consumption price would indeed mean that the device consumes without accruing costs, and a zero device production price would mean that the device produces without accruing revenue. The zero price should not constraint their power flow. But their power flow may still lead to accruing costs or revenues in case there are non-zero prices on the grid.
Hope this clarifies things?
There was a problem hiding this comment.
Yes thanks @Flix6x.Things are clearer now.
I will change the dependency of the price sensors to 'd' instead then.
There was a problem hiding this comment.
Hi again @victorgarcia98 @Flix6x
Upon thinking it through even more, I realized that d index wont suffice for the situation where i have a building/load as one of my inflexible devices.
In this case,the prices for consumption are not to be set and are to be decided by the solver.
But according to what @Flix6x said we could either set the prices or set it to 0 while iterating through the inflexible devices.This wont work out here.
I really think there is the necessity for these new index dimensions,which otherwise would just complicate stuff.
We could get on a call if you are still not satisfied with the reasoning.
flexmeasures/data/tests/conftest.py
Outdated
| generic_asset_type=asset_type, | ||
| ) | ||
| db.session.add(Solar1) | ||
| testing_sensor1 = Sensor( |
There was a problem hiding this comment.
I suggest to use more self explanatory variable names. In this case, I would use solar1_production_price_sensor instead of testing_sensor1.
flexmeasures/data/tests/conftest.py
Outdated
| db, testing_sensor6, values, time_slots, setup_sources["Seita"] | ||
| ) # make sure that prices are assigned to price sensors | ||
| db.session.flush() | ||
| return { |
There was a problem hiding this comment.
To simplify, I would return just the sensors:
return testing_sensor1, testing_sensor2, ...Then, when using the fixture, you can unwrap the return variables:
testing_sensor1, testing_sensor2, ... = create_solar_plants
flexmeasures/data/tests/conftest.py
Outdated
| ) | ||
| add_as_beliefs(db, testing_sensor8, values, time_slots, setup_sources["Seita"]) | ||
| db.session.flush() | ||
| return { |
| 0.95, | ||
| ], | ||
| ) | ||
| def test_schedule( |
There was a problem hiding this comment.
I would use a different name for this test, something like test_schedule_multiple_price_sensors and update the docstring to reflect that this test is using different price sensors.
| down_efficiency=roundtrip_efficiency**0.5, | ||
| decimal_precision=5, | ||
| ) | ||
| # Check if constraints were met |
There was a problem hiding this comment.
Still, this is not checking that the impact of having different sensors in the final schedule.
What about using different price sensor for the storage in two different schedule runs and then compare the two schedules? What do you think @Flix6x ?
There was a problem hiding this comment.
Still, this is not checking that the impact of having different sensors in the final schedule.
What about using different price sensor for the storage in two different schedule runs and then compare the two schedules? What do you think @Flix6x ?
Also what can be done for this?
There was a problem hiding this comment.
You could come up with two example problems with expected optimal schedules. Maybe including assert statement against the total costs of the two schedules (by multiplying the resulting schedule against corresponding prices), or asserting that a certain effect can be observed in the schedule at a certain time. The two problems could differ in terms of e.g. the size of the PV subsidy, such that with a high subsidy the schedule can be seen to favor feeding PV into the grid, whereas with a low subsidy, the schedule could favor self-consumption instead.
I'm not sure if this particular example makes sense in the context of your use case, but there must be some effect you are hoping to accomplish by setting individual device prices, right? For now this test only checks whether the resulting schedule respects the power constraints and SoC constraints, which does not yet constitute a test of the new functionality.
There was a problem hiding this comment.
If you need to consult further before being able to expand this test, please create a drawing with example numbers for power and price values, and expected costs.
|
closes #618 |
|
Sure @victorgarcia98 |
|
Hey @Flix6x @victorgarcia98 |
|
Hi @victorgarcia98 and @Flix6x, any chance you got an opportunity to review the latest edits? Could we connect offline to discuss modifications that you think need to be made? Thanks! |
|
Hey @victorgarcia98 and @Flix6x |
There was a problem hiding this comment.
Running make test is a good first step in showing backwards compatibility. I ran it, but tests are failing, most of them (but not all) on an IndexError in the price_up_select method:
def price_up_select(m, d, c, j):
> return commitment_upwards_deviation_price_array[d][c].iloc[j]
E IndexError: list index out of range
| 0.95, | ||
| ], | ||
| ) | ||
| def test_schedule_multiple_price_sensors( |
There was a problem hiding this comment.
This test seems to be failing with IndexError: list index out of range. Did you run pytest?
| down_efficiency=roundtrip_efficiency**0.5, | ||
| decimal_precision=5, | ||
| ) | ||
| # Check if constraints were met |
There was a problem hiding this comment.
If you need to consult further before being able to expand this test, please create a drawing with example numbers for power and price values, and expected costs.
Based on issue #618 and discussion #615