-
Notifications
You must be signed in to change notification settings - Fork 9
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 critical demand #165
base: dev
Are you sure you want to change the base?
Conversation
Hi @adnanalakori! I edited the first post according to what is common use in github. There is a specific way to reference the issues that you are working on with a hashtag, and the to-do-list are clickable. It may be that you do not want to adress each of tasks in this PR. Then delete some of the rows again. The PR can be closed if you can make sure that your changes do not interfere with the existing functionalities. |
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.
You did the first steps to implement the timeseries, let me know if you have trouble following though the next steps!
One remark: Your excel file as it is would not be helping you to implement the feature, as it calls column Demand
four times
Instead of calling demand_ac_non_critical
etc. which are the actual column heads of your demand profiles in inputs/timeseries/test_site.csv
.
Hi @smartie2076 , thanks! I unified names Line 45 in a8fa6f1
The simulation up to now runs well and checked in simulation_experiments.csv .
|
Can you upload the I wonder if it did load the timeseries of I think we might need to add it to (I am not sure if the following two points are really checked, or if some more work has to be done)
|
Regarding the next point, I did not consider it as the new parameters have been shown in simulation_experiments.csv
Also same apply to this point -> I understood "parsed correctly" that the simulation run with no errors:_
|
You do not have to quote reply all of it, just the parts you want to hightlight ;) Ah, I realized that actually we can not figure out if it worked with I am sure, actually, that we have to adapt this function:
There should definitly be more logging messages. So, try to understand the idea of the function and add some logging messages that help you understand which dara is added to the input data here. To observe what is changed in the function, you can add
Right below Line 117 in 4d21c9e
You can also check if you can store the
|
If you choose to improve the offgridders code and go for...
You should push the related commits in a seperate PR, ideally WITHOUT the |
4d21c9e
to
1ca9b2e
Compare
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 @adnanalakori,
I added some comments in the code below. It is very important in python not to make the code more bulky and especcially important to try and avoid code duplications - write new functions instead, that you call multiple times. That way, you can also write unit tests for them and figure out if they work.
There are some logical errors, but I know that Offgridders is just big, it is hard to know what is going into where. Take special care when you change if-loops, and read the existing logging
messages to figure out how the code should be changed.
I recommend you to add more logging.debug
messages to your code to make sure that your changes make sense.
Did you check the experiment
dict for the new information on the critical demand, and if it is parsed? This is very important, you need to know your code does what it should do before you go further and change stuff in other, later modules.
I think you error message might have to do with the fact that you completely removed the normal demand profile, and Offgridders can not parse the new critical ones jet. That is just a guess. If it persists, this PR is the right place to paste the full error message (not as a screenshot, either as a code or, if too long, as a txt).
/.idea/ |
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 file should not be commited.
@@ -0,0 +1,28 @@ | |||
****************************************************** |
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 file should not be commited.
src/D0_process_input.py
Outdated
TOTAL_DEMAND_AC_CRITICAL, | ||
PEAK_DEMAND_AC, | ||
PEAK_DEMAND_AC_CRITICAL, | ||
TOTAL_DEMAND_DC, | ||
TOTAL_DEMAND_DC_CRITICAL, | ||
PEAK_DEMAND_DC, | ||
PEAK_DEMAND_DC_CRITICAL, | ||
PEAK_PV_GENERATION_PER_KWP, | ||
PEAK_WIND_GENERATION_PER_KW, | ||
MEAN_DEMAND_AC, | ||
MEAN_DEMAND_AC_CRITICAL, | ||
MEAN_DEMAND_DC, | ||
MEAN_DEMAND_DC_CRITICAL, | ||
PEAK_MEAN_DEMAND_RATIO_AC, | ||
PEAK_MEAN_DEMAND_RATIO_AC_CRITICAL, | ||
PEAK_MEAN_DEMAND_RATIO_DC, | ||
PEAK_MEAN_DEMAND_RATIO_DC_CRITICAL, | ||
ABS_PEAK_DEMAND_AC_SIDE, | ||
ABS_PEAK_DEMAND_AC_CRITICAL_SIDE, |
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 is an executive decision - it seems you want to calculate the indicators for every demand type, an not aggregated? Fine by me, makes sense, but we will have to make sure that these parameters are also printed to the results file. This happens far at the end of the Offgridders process, though. Keep this comment open in Github as a reminder.
src/G1_oemof_create_model.py
Outdated
# ------------demand sink ac critical ------------# | ||
demand_ac_critical = generate.demand_ac_critical( | ||
micro_grid_system, bus_electricity_ac, experiment[DEMAND_PROFILE_AC] | ||
) | ||
|
||
# ------------demand sink dc critical ------------# | ||
demand_dc_critical = generate.demand_dc_critical( | ||
micro_grid_system, bus_electricity_ac, experiment[DEMAND_PROFILE_DC] | ||
) |
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.
You are defining new functions demand_ac_critical
and demand_dc_critical
here. However, those functions are absolutely identical - apart from the input argument of the demand. Create one single function for all these instances, ie. use a new function generate.demand()
four times, for demand_ac, demand_dc, demand_ac_critical, demand_dc_critical
. That is the pythonic way.
@@ -745,6 +746,21 @@ def maingrid_feedin(micro_grid_system, experiment): | |||
) | |||
}, | |||
) | |||
|
|||
def demand_dc_critical(micro_grid_system, bus_electricity_dc, demand_profile): |
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.
As described above, generalize these functions:
def demand(micro_grid_system, bus_electricity, demand_profile, profile_title):
...
Hi @smartie2076 ,
|
Hi @adnanalakori! Thanks for posting the issue here - it makes it easier to follow up on. Ideally, when posting issues here, you add links to the appropriate code passages, so that it is absolutely without possibility of error clear which lines of code you are referring to. For that, you open the file in github (here I add the links below, so that we are for sure talking about the same thing.
We are talking about this function: offgridders/src/D0_process_input.py Line 361 in ab33623
Thing is, it is not a proper function definition - you can (almost) only define functions on the first level (no tabs in front of
So, move this function out of your if-else-loop, and only call the function within this if-else-loop. Otherwise, the function is not recognized.
Line 476 would be this, so I guess the line where you got an error. offgridders/src/D0_process_input.py Line 476 in ab33623
I think the problem is that you removed 4 similar code snippets and replaceAd them by |
Means adding constraint that allows capacity dispatch to supply critical load first and then non-critical load after that? The constraints shall work as a check point, correct? |
I assumed that the amount of “non-critical demand is not covered” is already fixed at input constant as scenarios e.g. 10% 30% etc.? Do we still need to apply a KPI here? |
I assumed that the critical demand always supplied. Do we still to implement something here as well? could you elaborate on? |
Regarding model inputs and outputs, I could imagine the followings: Model inputs:
Model outputs:
|
Means that the shortage that is allowed can only be as high as non-critical load only, so that critical load is always supplied. The constraints are defined in The constraint offgridders/src/G2b_constraints_custom.py Line 959 in 76173e0
Is not implemented or used, so do not just change that, it will not help. However, it also seems to me that the "timestep" constraint is anyway added with
Sidenote, can also be moved into an own issueI would greatly appreciate if this PR could do a fix and rename the variable Line 152 in 76173e0
I am not sure how this came to be, but it is very, very misleading in offgridders/src/G2b_constraints_custom.py Line 64 in 76173e0
|
Yeah, if you implement is correctly it is. But there are some parameters/KPI, that should take this into account - for example the % of shortage should then not be calculated based on |
That is the idea.
You have to implement this into the plots, only if you do it will be plotted.
Some KPI calculations might have to be changed, as they have the demand as a denominator, which currently is only the non-critical demand. |
This would be added in G2b and called under |
Yeah it would. I am just not sure if it is basically already in the different stability constraints that I defined, or if it is not. Stability constraints would be: offgridders/src/G2b_constraints_custom.py Line 52 in 76173e0
offgridders/src/G2b_constraints_custom.py Line 242 in 76173e0
offgridders/src/G2b_constraints_custom.py Line 431 in 76173e0
They are always written with a validity-check that has to pass after the optimization finished, eg: offgridders/src/G2b_constraints_custom.py Line 167 in 76173e0
You would call the constraint in offgridders/src/G1_oemof_create_model.py Line 514 in 76173e0
|
Create a function to update the demand and not repeat code
Not exactly sure it is the right implementation, need to check that and modify if needed
8c3ac8a
to
16a676d
Compare
src/G2b_constraints_custom.py
Outdated
""" | ||
|
||
def meet_critical_demand_rule(model): | ||
# TODO express the fact that the critical demand must be met as a constraint |
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.
@smartie2076 - would something like this work? (Provided we then define total_production
with arguments form the function
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.
Yeah, this should work.
) | ||
|
||
stability_constraint = case_dict.get(STABILITY_CONSTRAINT, None) | ||
if stability_constraint == CRITICAL: |
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 am not sure if this was the correct way to go with the shortage for critical demand @smartie2076
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 am having issues with the naming here. stability_constraint
should be a simplified electro-technical stability constraint, but here you are not assessing stability, but more something like a load_shedding
option.
Also, here, you access the same stability_constraint
value:
offgridders/src/G1_oemof_create_model.py
Line 406 in d7881b0
# ------------Stability constraint------------# |
But this stability constraint is, for me, not the same as the load shedding option. Actually, as soon as you define non-critical demand, load shedding will per definitionem be allowed. This is also not very beautiful, but okay, as it is within the naming. You do not have to access the STABILITY_CONSTRAINT
value for this.
I am not sure if allowing for load shedding will require all three stability constraints (share_backup, share_hybrid, share_usage
) to be re-defined. Because, if there is intentional load-shedding, then it should be okay that also only a relative part of the remaining demand is covered by renewables or whatever. However, if 100% demand (critical+non-critical) is supplied, the share of renewables or whatever should be based on that effective demand. Meaning, all four (AC/DC critical/non-critical) demands need to be in the constraint - maybe the shortage asset then plays the deciding role here?
About shortage: I do not remember if this is the only part where it is implemented. In general, one needs to decide, whether load shedding of non-critical loads is considered shortage or not. I think right now, load shedding would be allowed within the margin of allowed shortage, effectively limiting its potential.
b94124e
to
16a676d
Compare
src/G0_oemof_simulate.py
Outdated
e_flows_df = timeseries.get_demand( | ||
case_dict, | ||
oemof_results, | ||
electricity_bus_ac, | ||
electricity_bus_dc, | ||
critical, | ||
experiment, | ||
) |
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.
@smartie2076 would that help for results plotting of the critical?
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 know, as I am not quite sure what your CRITICAL
is right now - I did not find it in the code apart from something, where I thought it was a string only, not something added to the oemof model itself. I also don´t know what you are planning to plot...
Did the code run though with this new code bit? What results/graph did it generate?
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 purpose is to separate, simulate and plot results of critical
demand. This new function still did not run it yet, we still not sure about it yet.
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 am sorry, I do not understand. I suggest that you create a blueprint plot with excel, to showcase what kind of graph you want to have generated.
Dont think I can help you here, though, as I am also not really in tune with the Offgridders code anymore.
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.
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 the "Demand" is supposed to be the total demand, including the DC and AC demand, and thus it should include critical demand as well. You just need to change the calculation of the "Demand". If you want to highlight that there is a specific (total) critical demand, I would suggest to add a dotted line of the same color as the total demand. The allowed shortage would then be the margin between those two lines.
src/G1_oemof_create_model.py
Outdated
# TODO maybe important for critical | ||
# AD: We might leave this certain of shortage to assign to the non critical demand? |
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.
Any suggestion here? @smartie2076
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 am sorry, I do not understand the question.
In general, the question about allowed shortage, reliability and shedded load ends up in this questio: Is load shedding of non-critical loads considered shortage? If it is, then load shedding must be limited to the allowed shortage as defined in the input template. The resulting reliability will most often be equal to the allowed shortage. If load shedding is not considered shortage, I think it would still make sense that the reliability is then lower than if no load was shedded - so the above calculation should reflect that.
I suggest writing pytests or benchmark tests to make sure the feature is working properly and continues working properly in the future.
src/G1_oemof_create_model.py
Outdated
# TODO maybe important for critical | ||
# AD: We might leave this certain of shortage to assign to the non critical demand? | ||
""" | ||
# ------------Allow shortage only for certain percentage of demand in a timestep------------# | ||
if case_dict['allow_shortage'] is True: |
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.
Any suggestion here? @smartie2076
src/G1_oemof_create_model.py
Outdated
# TODO maybe important for critical | ||
# AD: We might leave this certain of shortage to assign to the non critical demand? | ||
""" | ||
# ------------Allow shortage only for certain percentage of demand in a timestep------------# | ||
if case_dict['allow_shortage'] is True: |
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.
Any suggestion here? @smartie2076
src/G1_oemof_create_model.py
Outdated
elif case_dict[STABILITY_CONSTRAINT] == CRITICAL: | ||
# ADDED: a function to make the stability constraint favor critical demand over demand | ||
logging.info( | ||
"Added constraint: Stability though actual generation of diesel generators and backup through batteries." | ||
) | ||
constraints_custom.critical( | ||
model, | ||
case_dict, | ||
experiment=experiment, | ||
storage=storage, | ||
sink_demand=sink_demand_ac, | ||
genset=genset, | ||
pcc_consumption=pointofcoupling_consumption, | ||
source_shortage=source_shortage, | ||
el_bus_ac=bus_electricity_ac, | ||
el_bus_dc=bus_electricity_dc, |
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 am trying to add a function to make the stability constraint in favor critical demand, not sure!
I just noticed that you should make sure that your code still works with the old input template - ie. that if |
The aim is to compute the energy balance between production and consumption of each AC and DC busses and make sure there is enough energy to fullfill criticial demand at all timesteps
d7881b0
to
271b4ac
Compare
The triggering by critical constraint was wrong
TODO: adapt the code so that users not using critical demand do not recieve errors
if the critical demand option is on
Also move the color dict as a constant in G4 module
Work in progress to implement https://forum.openmod.org/t/define-shortage-in-oemof/2919/5
Adresses issue #162
The following subtasks have been implemented:
Defined
title_demand_ac_critical
andtitle_demand_dc_critical
in the excelDefine constants in
offgridders/src/constants.py
Line 45 in a8fa6f1
Added columns for
demand_ac_critical
anddemand_dc_critical
in input timeseries fileRun simulation, check that new parameters are in
simulation_experiments.csv
(output)If not, needs to be implemented in
D0
https://github.com/rl-institut/offgridders/blob/dev/src/D0_process_input.pyCheck that timeseries was parsed correctly, add
DEMAND_PROFILE_AC_CRITICAL
andDEMAND_PROFILE_DC_CRITICAL
Add
generate_demand
for new critical demands, seeoffgridders/src/G1_oemof_create_model.py
Line 96 in a8fa6f1
--- For later ---
Run a simulation with demands split into normal and critical demand to compare if KPI like
demand_total
stay the same (do not use shortage here), and also which do not. We might have to adapt KPI definitions accordingly (G3
,G3a
, G3b`)Add a constraint that allows shortage to replace non-critical demands (
G2b
)Think of KPI that represents how much non-critical demand is not covered. Should this be in
total_shortage
etc?Add plausability tests, ie. is critical demand always supplied, and is the shortage activated ADDITIONALLY? (
G3a
)Define benchmark test, ie. scenarios where you already can estimate some results and with which we can test that the new feature works.
Make sure that all excel files in the repo now are up-to-date and have the new parameters
Make sure that it is possible to parse excel files that do not have the new parameters. Give a warning about outdated input files.
Update the
changelog.md
Use
black
for linting