Ability to add buses to the grid via the Change Table #352
Conversation
| if l not in new_bus.keys(): | ||
| raise ValueError(f"Each new bus needs {l} info") | ||
| if not isinstance(new_bus[l], (int, float)): | ||
| raise ValueError(f"{l} must be numeric (int/float)") |
There was a problem hiding this comment.
I never know about these ones. The input is of the right type, but the values within it are the wrong type. I have no idea what is 'right'.
There was a problem hiding this comment.
Thinking a little bit more about it. I would say the the first exception should be a KeyError since, after all, if you try to access a non-existing key in a dictionary this would be the error raised. I still think that the second one should be a TypeError. Let's see what @BainanXia and @jon-hagg think about it.
There was a problem hiding this comment.
I would vote for either TypeErorr or ValueError. KeyError seems like an implementation detail, which the caller of the function should not need to be aware of. We check the inputs so that we don't end up throwing the KeyError, and can raise something more meaningful to the calling function/user.
There was a problem hiding this comment.
@rouille I agree with you, the first one should be a KeyError (Python catches keyerror itself by default) given the fact the code is trying to find an expected key in a dict but fails and the second one should be a TypeError given it is thrown after a failure of isinstance check.
There was a problem hiding this comment.
That is what we're doing in:
for l in {"lat", "lon"}:
if l not in new_bus.keys():
raise ValueError(f"Each new bus needs {l} info")
We don't explicitly say that {"lat", "lon"} is the mandatory set, but that's what it is. We could make the code more explicit about that.
There was a problem hiding this comment.
zone_id or zone_name is also a mandatory key. Anyway, it is not very important and checking for the completeness of the dictionary and raise an error if not is good.
There was a problem hiding this comment.
We are checking that one and only one of these are specified via
if {"zone_id", "zone_name"} <= set(new_bus.keys()):
raise ValueError("Cannot specify both 'zone_id' and 'zone_name'")
if {"zone_id", "zone_name"} & set(new_bus.keys()) == set():
raise ValueError("Must specify either 'zone_id' or 'zone_name'")I guess we could make that simpler/clearer, e.g.
if len({"zone_id", "zone_name"} & set(new_bus.keys())) != 1:
raise ValueError("Must specify either 'zone_id' or 'zone_name' (but not both)")There was a problem hiding this comment.
@danielolsen, feel free to raise whatever exception you want. It is a detail and we should move on. Don't forget to document the docstring accordingly.
There was a problem hiding this comment.
It is documented.
| del new_bus["zone_name"] | ||
| if "Pd" in new_bus: | ||
| if not isinstance(new_bus["Pd"], (int, float)): | ||
| raise ValueError("Pd must be numeric (int/float)") |
| new_bus["Pd"] = defaults["Pd"] | ||
| if "baseKV" in new_bus: | ||
| if not isinstance(new_bus["baseKV"], (int, float)): | ||
| raise ValueError("baseKV must be numeric (int/float)") |
|
Should we update the |
My first instinct: I don't want to. My second instinct: I think you are right. We don't use this in many places, but because of how we are interpreting the grid.mat files, I think we need We'll want to make sure to do some end-to-end testing to make sure that this not only builds a Grid properly, but that it can be loaded successfully as a ScenarioGrid. |
Agree. I'm about to point it out that the modified grid not only matters when building it but also loading it. We would like to have a consistent dataframes everywhere for a specific scenario. You've been faster than me. |
|
I added a new test which detects this failure. We will need to modify |
|
Substations are now added automatically, as needed, and this is checked in the
EDIT: here's what we get with |
e889cbf to
cc749de
Compare
|
This has been refactored to be more performant: inspired by @jon-hagg, I added a caching method in Test times are back down to 11-12 seconds on my machine. |
|
This has been end-to-end tested on a scenario, adding a branch, a plant, ad a storage device to a new bus. The scenarios prepares, runs, and extracts properly, all from within PowerSimData. On the way, I found a bug in how we prepare profiles: previously, we would only use TransformProfile if the profiles were being scaled, but not if only new plants were added. This was causing errors within REISE.jl because the case.mat file would list more generators than were in the profile. This has been fixed, see the new changes to The scenario setup used to test: from powersimdata.scenario.scenario import Scenario
scenario = Scenario('')
scenario.state.set_builder(["Texas"])
scenario.state.builder.set_base_profile("demand", "ercot")
scenario.state.builder.set_base_profile("hydro", "v2")
scenario.state.builder.set_base_profile("solar", "v4.1")
scenario.state.builder.set_base_profile("wind", "v5.1")
scenario.state.builder.set_name("test", "new_bus2")
scenario.state.builder.set_time("2016-01-01 00:00:00", "2016-01-03 23:00:00", "24H")
new_bus_id = scenario.state.get_grid().bus.index.max() + 1
scenario.state.builder.change_table.add_bus(
[{"lat": 30, "lon": -95, "zone_id": 308}]
)
scenario.state.builder.change_table.add_storage_capacity(
bus_id={new_bus_id: 100}
)
scenario.state.builder.change_table.add_plant(
[{"type": "wind", "bus_id": new_bus_id, "Pmax": 400}]
)
scenario.state.builder.change_table.add_branch(
[{"from_bus_id": (new_bus_id - 1), "to_bus_id": new_bus_id, "capacity": 300}]
)
scenario.state.create_scenario()
scenario.state.prepare_simulation_input()
scenario.state.launch_simulation(threads=8) # By default will auto-extractThen, loading the completed scenario is successful: >>> scenario = Scenario(1713)
Transferring ScenarioList.csv from server
100%|########################################| 234k/234k [00:00<00:00, 851kb/s]
Transferring ExecuteList.csv from server
100%|######################################| 20.7k/20.7k [00:00<00:00, 175kb/s]
SCENARIO: test | new_bus2
--> State
analyze
--> Loading grid
1713_grid.mat not found in C:\Users\DanielOlsen\ScenarioData\ on local machine
Transferring 1713_grid.mat from server
100%|#######################################| 191k/191k [00:00<00:00, 1.24Mb/s]
Loading bus
Loading plant
Loading heat_rate_curve
Loading gencost_before
Loading gencost_after
Loading branch
Loading sub
Loading bus2sub
--> Loading ct
1713_ct.pkl not found in C:\Users\DanielOlsen\ScenarioData\ on local machine
Transferring 1713_ct.pkl from server
100%|#########################################| 368/368 [00:00<00:00, 3.57kb/s] |
Good catch! Tested on my end and it works! |
406b678 to
5726464
Compare
2283056 to
3178401
Compare
3178401 to
6860791
Compare
6860791 to
7c60fbe
Compare
Purpose
Adding buses via the change table will allow us greater flexibility in constructing scenarios, e.g. modeling 'hybrid' power plants which feature a generator and a storage device behind a single point-of-interconnection with the rest of the grid.
Closes #341.
What the code is doing
In change_table.py:
ChangeTable.add_busmethod. This is modeled after the dcline/plant additions, where it's passed a list of dicts, it validates each one, and eventually adds them to thectdict, translating fromzone_nametozone_idas necessary.ChangeTable.{add_storage_capacity, add_plant, _add_line}functions to check if the new assets they are adding are being added not only at the locations of existing grid buses, but expanding this check to include the new buses as well.haversinefunction and checking if the result is zero (should be equivalent but simpler). This is unrelated, but I noticed that it could be simpler so I decided to refactor it.In transform_grid.py:
TransformGrid._add_busmethod to interpret the"new_bus"keys in a ChangeTable, and running this as a part ofTransformGrid.get_grid().Testing
New unit tests:
Time estimate
An hour. Most of the code is in a few big, pretty straightforward chunks, but there are also some smaller changes scattered around, and given the importance of the TransformGrid object, we want to make sure we're not introducing any regressions or new edge cases.