Lossy Bidirectional Links #1192
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -265,6 +265,36 @@ def attach_hydrogen_pipelines(n, costs, config): | |
| carrier="H2 pipeline", | ||
|
There was a problem hiding this comment.
|
||
| ) | ||
|
|
||
| # setup pipelines as bidirectional and lossy | ||
| lossy_bidirectional_links(n, "H2 pipeline") | ||
|
There was a problem hiding this comment. Based on all my suggestions, this would be change to: And in the |
||
|
|
||
|
|
||
| def lossy_bidirectional_links(n: pypsa.components.Network, carrier: str): | ||
|
There was a problem hiding this comment. I think that it is better to move Additionally, I would prefer to apply directly the length based pipeline efficiency that I saw in PR #1215. Consequently changing the function to the following similarly to PyPSA-Eur but without the .location at the moment: As for the If this is done as above, then PR #1215 is not needed anymore and issue #1213 can be closed. There was a problem hiding this comment. Great idea, to move the function to helpers! I think it's nice to implement both PR's together, but I'd prefer to leave the functions seperate (eventhough PyPSA-Eur combines them) to make them more reusable. I think it's quite plausible, that some new components might be bidirectional and lossy, but without a length-based efficiency, or have a length-based efficiency, but are only unidirectional. There was a problem hiding this comment. @Eddy-JV regarding the efficiency_per_1000km you provided: There was a problem hiding this comment. @Eric-Nitschke Thank you for catching that. In the Hydrogen case the efficiency should be |
||
|
|
||
| "Split bidirectional links into two unidirectional links to include transmission losses." | ||
|
Comment on lines
+273
to
+274
There was a problem hiding this comment. Usually, we are using |
||
|
|
||
| carrier_i = n.links.query("carrier == @carrier").index | ||
|
|
||
| if carrier_i.empty: | ||
| return | ||
|
|
||
| logger.info(f"Splitting bidirectional links with the carrier {carrier}") | ||
|
|
||
| n.links.loc[carrier_i, "p_min_pu"] = 0 | ||
|
|
||
| rev_links = ( | ||
| n.links.loc[carrier_i].copy().rename({"bus0": "bus1", "bus1": "bus0"}, axis=1) | ||
| ) | ||
| rev_links["length_original"] = rev_links["length"] | ||
| rev_links["capital_cost"] = 0 | ||
| rev_links["length"] = 0 | ||
| rev_links["reversed"] = True | ||
| rev_links.index = rev_links.index.map(lambda x: x + "-reversed") | ||
|
|
||
| n.links = pd.concat([n.links, rev_links], sort=False) | ||
| n.links["reversed"] = n.links["reversed"].fillna(False).infer_objects(copy=False) | ||
| n.links["length_original"] = n.links["length_original"].fillna(n.links.length) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| if "snakemake" not in globals(): | ||
|
There was a problem hiding this comment. Please accomodate for override components by: Removing: Adding: There was a problem hiding this comment. Hey @Eddy-JV, frankly speaking I'm not quite aware of the proper approach to overwrite attributes. I assume the need arises from the need to translate a power-only model into the sector-coupled version, but having a bit more details would be appreciated 🙂 There was a problem hiding this comment. Hey @ekatef, in this case the need arises from adding a compression electricity demand to the pipelines. This requires a third bus (bus2) for the link and a corresponding efficiency (efficiency2). The override_component_attr function adds these fields (and more) to the network. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -854,6 +854,43 @@ def add_existing(n): | |
| n.generators.loc[tech_index, tech] = existing_res | ||
|
|
||
|
|
||
| def add_lossy_bidirectional_link_constraints(n: pypsa.components.Network) -> None: | ||
|
There was a problem hiding this comment. I did this slightly different (See below). Feel free to check and comment if you do not agree: There was a problem hiding this comment. Hey @Eddy-JV, even though the additional checks you proposed here are not neccessary right now, I think it's great to future-proof the code right now. I'm going to include the additional checks in the final version. |
||
| """ | ||
|
Comment on lines
+857
to
+858
There was a problem hiding this comment. Types hinting is generally a great idea, but we don't use. I'd suggest to remove it here for consistency. Happy to discuss an overall revision to implement type hinting across the whole codebase if feels handy There was a problem hiding this comment. Agree. Maybe we can put this as a seperate issue if needed. |
||
| Ensures that the two links simulating a bidirectional_link are extended the same amount. | ||
| """ | ||
|
|
||
| if not n.links.p_nom_extendable.any() or "reversed" not in n.links.columns: | ||
| return | ||
|
|
||
| n.links["reversed"] = n.links.reversed.fillna(0).astype(bool) | ||
| carriers = n.links.loc[n.links.reversed, "carrier"].unique() # noqa: F841 | ||
|
There was a problem hiding this comment. I understand that Looking into flake8 error codes, it appears that There was a problem hiding this comment. Short answer: carriers is used in the lines just below, but the interpreter doesn't realize it, because its used in with .query(). Thats why noqa: F841 is used. I'll write a long answer tomorrow. There was a problem hiding this comment. Slightly longer answer: uses carriers, but because pandas.DataFrame.query() takes a string as an input, the python interpreter doesn't realize that carriers is used via the "@carriers". I don't think it is necessary to use .query() here. Instead, we could use .loc[]: In general, I think we should discuss whether we should phase out the use of .query() and replace it with .loc[] for all of PyPSA-Earth to make it more consistent and readable. There was a problem hiding this comment. Ahh, ok! Thanks a lot for investigating this! Agree with the proposal on replacing here Regarding the overall code refactoring, totally agree that there is a room for improvement and you are very welcome to contribute there. Frankly speaking, I'm not ready to confirm that |
||
|
|
||
| forward_i = n.links.query( | ||
| "carrier in @carriers and ~reversed and p_nom_extendable" | ||
| ).index | ||
|
|
||
| def get_backward_i(forward_i): | ||
| return pd.Index( | ||
| [ | ||
| ( | ||
| re.sub(r"-(\d{4})$", r"-reversed-\1", s) | ||
| if re.search(r"-\d{4}$", s) | ||
| else s + "-reversed" | ||
| ) | ||
| for s in forward_i | ||
| ] | ||
| ) | ||
|
Comment on lines
+872
to
+882
There was a problem hiding this comment. Just for better understanding, what is a particular reason to apply here regular expressions? Again, I understand that implementation follows PyPSA-Eur and I'm perfectly fine with leaving it there. Just trying to draft some approach to further improvements which we could probably add as TODO if it would make sense 🙂 I guess Regular expressions are There was a problem hiding this comment. Regular expression enables it us to easily change indices tha end in a four digit number to not end in reversed but have it in the middle instead. I.e. "H2 pipeline DZ0 0-DZ0-1234" to "H2 pipeline DZ0 0-DZ0-reversed-1234" instead of "H2 pipeline DZ0 0-DZ0-1234-reversed". If we assume that the four digit ending can be ignored, the whole thing could be replaced with something like backward_i = forward_i + "-reversed" or backward_i = forward_i.apply(lambda x: f"{x}-reversed"). There was a problem hiding this comment. Thanks a lot for the explanations. Do you see any advantages in having There was a problem hiding this comment. @ekatef I don't see any advantage to it. |
||
|
|
||
| backward_i = get_backward_i(forward_i) | ||
|
|
||
| lhs = linexpr( | ||
| (1, get_var(n, "Link", "p_nom")[backward_i].to_numpy()), | ||
| (-1, get_var(n, "Link", "p_nom")[forward_i].to_numpy()), | ||
| ) | ||
|
|
||
| define_constraints(n, lhs, "=", 0, "Link-bidirectional_sync") | ||
|
|
||
|
|
||
| def extra_functionality(n, snapshots): | ||
| """ | ||
| Collects supplementary constraints which will be passed to | ||
|
|
@@ -881,6 +918,7 @@ def extra_functionality(n, snapshots): | |
| if "EQ" in o: | ||
| add_EQ_constraints(n, o) | ||
| add_battery_constraints(n) | ||
| add_lossy_bidirectional_link_constraints(n) | ||
|
|
||
| if ( | ||
| snakemake.config["policy_config"]["hydrogen"]["temporal_matching"] | ||
|
|
||
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 guess those are changes from an automated run of github workflow? 😄 Don't think we need them in the PR. Could you please remove them?
The recent PR #1210 changes a mode of the automated updates of the contributors list to a pre-scheduled one. So we don't loose track of contributions but PRs won't have those irrelevant additions 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.
I agree.