Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(scheduling): Price devices distinctively in scheduling #654
base: main
Are you sure you want to change the base?
feat(scheduling): Price devices distinctively in scheduling #654
Changes from 21 commits
ab72c4f
a9550f0
bea996e
a80a475
014edfa
515c31b
c34952d
be79cf7
901b7f3
fc12afb
346030c
be387b5
f830cfc
e2fcb5a
7307256
0a9c6f0
9fe1831
a931838
f0919ae
41d3a2f
11d2b03
95da3f4
ea323eb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:{"a" : 2}
,a=1
,"a"=1
,a:1
or'{"a":2}'
.Test command:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctly said.I propose we do something like this?
Using this now I can use the format { "19" : 21}
Let me know if that works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toreplace_deprecated_argument
.Is that how it is supposed to be done?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.ecommitment_downwards_deviation_price
,commitment_upwards_deviation_price
). What do you think @Flix6x?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring is not updated with the new parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that instead of using
u
andp
indexes, the price should depend only on the device (d
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
In this example, we would have
consumption_price_sensor_per_device = [1,3,5]
andproduction_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My interpretation was on the basis of the assumption that a device may have only one price sensor attached to it.For example,
Let me know if my understanding is right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think? @victorgarcia98
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, we are on the same page!
Then, we can use the device index
d
(instead ofu
andp
) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @victorgarcia98 .That works.
But couple of things i needed to clarify before working it through:
Please let me know if you understood my concerns.Also let me know that you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_sensor
andproduction_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if
consumption_price_sensor
is provided instead ofconsumption_price_sensor_per_device
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, if the
StorageScheduler
is called through the api, it will fail.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Shall we just use if case to handle that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 sameconsumption_price_sensor
. The same for production.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need to map each device with
consumption_price_sensor
or could i just mapconsumption_price_sensor
with the battery sensor either?Shouldn't that work as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
If I am reading your suggestion right, you propose:
Example B
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow. Can you explain it with an example in more detail, with example values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier, we had the following:
which now has changed to this:
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 :This will make the situation exactly like Example A.There is no difference in the example.