-
Notifications
You must be signed in to change notification settings - Fork 14
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
Features/refactor check tech constraints #290
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.
Hi @birgits
thanks for the refactoring. Looks really good in general!
Still, I added some comments. Many of them are optional. We can also discuss if any of them are unclear.
@@ -2342,6 +2477,9 @@ def resample_timeseries( | |||
|
|||
""" | |||
self.timeseries.resample_timeseries(method=method, freq=freq) | |||
self.electromobility.resample(freq=freq) |
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 probably not part of this PR, but why are the names of the methods different, i.e. sometimes "resample_timeseries" and sometimes just "resample"?
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 right, I changed it.
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.
Is there a reason why this was moved to flex_opt? Intuitively, I would have looked in tools for it. Maybe even network or topology, but strictly speaking, it is not a flexibility option, is it?
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.
It has always been there.. The initial idea was, that it is needed to determine grid expansion needs. But you are right, it would also make sense in tools. I think it should be a separate PR though if we want to move it, because otherwise we can't see the changes done to the module in this PR.
lines_df.loc[lines_radial_feeders].s_nom / sqrt(3) / nominal_voltage, | ||
] | ||
) | ||
crit_lines_relative_load = relative_i_res[relative_i_res > 1].max().dropna() |
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 we really want to hard-code an allowed loading of 1? I know we always use it like this but I have seen papers where they allow more, e.g. 110%. Maybe we could have this as optional input and the default is 1?
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 it is correct like this because the relative loading is the ratio of allowed loading and actual loading. The allowed loading can be increased by changing the load factor in the config to e.g. 1.1 instead of 1. But the relative loading should never exceed 1.
""" | ||
Get allowed maximum current per line per time step | ||
Checks for over-loading issues of lines. |
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.
Maybe add:
"... and returns time and value of most severe overloading event per line"?
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 added it, thanks!
@@ -97,180 +95,240 @@ def lv_line_load(edisgo_obj): | |||
return crit_lines | |||
|
|||
|
|||
def lines_allowed_load(edisgo_obj, voltage_level): | |||
def _line_overload(edisgo_obj, voltage_level): |
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.
Maybe "_line_max_overload"? Otherwise I would expect all overloading events.
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 changed it.
raise ValueError("Inserted grid is invalid.") | ||
|
||
# get maximum allowed apparent power of station in each time step | ||
s_station = sum(transformers_df.s_nom) |
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.
Same comment as the one before (line 503).
|
||
upper_limit = ( | ||
1 | ||
+ offset |
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 I completely understood this parameter, but if I understood correctly, the sign should be reversed as it is always a positive value and a possible negative effect should be displayed here. If it should reduce the available voltage limit, then it has to be - here and + in line 949. But could also be that I have a twist in my head. Maybe you could check again, if it is correct the way it is implemented?
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 absolutely right, thanks! I corrected it.
|
||
|
||
def _lv_allowed_voltage_limits(edisgo_obj, lv_grid, mode): | ||
def allowed_voltage_limits(edisgo_obj, buses=None, split_voltage_band=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.
Again, I was asking myself if the voltage_level should be included here as well. I guess it is a bit confusing to me that some of the methods do include the voltage level and others then do not. This leads to some parts of the code not really being used in some of the cases but still being called. However, low priority I would say.
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.
Again you are right, but again I think it is not faster to check first for which voltage levels the functions should be called than simply calling them. Also, all public functions do not have the voltage level as parameter, but the lines or buses the user is interested in. The only exception being the function voltage_issues(), which is the equivalent to mv_line_overload and lv_line_overload. We could also split the voltage_issues() functions into mv and lv, to make it more consistent? But maybe the idea from above to have those functions that are very specific to the grid reinforcement in another module than the more general functions to obtain allowed load etc. would also make it more consistent?
secondary_sides = pd.Series() | ||
for grid in lv_grids: | ||
secondary_side = grid.station.index[0] | ||
if secondary_side in buses_in_pfa: |
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 would be the "else" here? Should the user be informed or is this nothing of interest?
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.
If there are no powerflow results for a bus you want to obtain the allowed voltage deviation for, the debug message in allowed_voltage_limits() will let you know. Maybe debug is not the right level and an error should be raised?
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 changed the logging level to debug.
@@ -133,12 +133,12 @@ def _station_overloading(edisgo_obj, critical_stations, voltage_level): | |||
dict |
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.
Could we change the name of this method into _reinforce_station_overloading instead of _station_overloading? Otherwise it sounds like you get the overloading.
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.
Done!
# Get allowed current per line per time step | ||
i_lines_allowed = check_tech_constraints.lines_allowed_load(edisgo_obj, "mv") | ||
i_lines_pfa = edisgo_obj.results.i_res[grid.lines_df.index] | ||
def _scored_critical_loading(edisgo_obj): |
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 not new with this PR but why do we only check lines here? Would transformers not be of interest 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 is because only MV is considered.
Description
The functions in check_tech_constraints are refactored so that it is easier to determine relative loading of all components and voltage deviation at all nodes. At the same time, the functions become more consistent. It is now possible to get allowed loadings and voltages, relative loadings and voltage deviations, and maximum overloadings and voltage issues.
Type of change
Checklist:
pre-commit
hooksNew and adjusted code includes type hinting now