-
Notifications
You must be signed in to change notification settings - Fork 209
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
Lossy Bidirectional Links #1192
Conversation
Summary: - fixed an existing bug regarding bidirectional lossy links that affected hydrogen pipelines. - the fix primarily utilizes code already in use by PyPSA-Eur. Changes: - added a function to scripts/add_extra_components.py that "splits" bidirectional links into two unidirectional links. This is based upon the similar PyPSA-Eur implementation, which is slightly more advanced to accommodate PyPSA-Eurs more advanced efficiency calculations. - added a function to scripts/solve_network.py that adds an additional constraint to the network, which ensures that both unidirectional links share the same nominal power. This is a modified version of the PyPSA-Eur function, as PyPSA-Eur uses linopy while the base PyPSA-Earth version operates on lopf. Reason: - without this commit, lossy bidirectional links are able to "generate" power by operating at a negative power level. - this stems from the general link implementation, which applies p1 = -eta*p0 to links, regardless whether power flows from bus0 to bus1 or from bus1 to bus0. - the current hydrogen pipeline implementation adds them to the network as lossy bidirectional links and is thus able to "generate" hydrogen. - this might affect other lossy bidirectional components added by the sector network as well.
for more information, see https://pre-commit.ci
Many thanks @Eric-Nitschke :D That's a quite neat application! |
Changes: - add a describtion of this pull request to the release_notes.rst
@davide-f Thanks :) |
- correct spelling mistakes in doc/release_notes.rst
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.
Hello @Eric-Nitschke, thanks a lot for investigating the issue and contributing the fix!
Have added some technical comments mainly aimed at better understanding and maintainability improvements. I recognise that the PR follows PyPSA-Eur implementation which is more or less state-of-the-art, but it would be definitely great to have deeper understanding of possible further improvements on some weak points. Your insights in this regard would be very helpful.
Personally, I think that this PR can be probably merged as-is as it feels like a much needed bug-fix. It would be great to have a review of @Eddy-JV who has been working on implementing similar solutions in PyPSA-Eur and definitely has a better overview
README.md
Outdated
<td align="center"> | ||
<a href="https://github.com/euronion"> | ||
<img src="https://avatars.githubusercontent.com/u/42553970?v=4" width="100;" alt="euronion"/> | ||
<br /> | ||
<sub><b>Euronion</b></sub> | ||
</a> | ||
</td> | ||
<td align="center"> | ||
<a href="https://github.com/Justus-coded"> | ||
<img src="https://avatars.githubusercontent.com/u/44394641?v=4" width="100;" alt="Justus-coded"/> | ||
<br /> | ||
<sub><b>Justus Ilemobayo</b></sub> | ||
</a> | ||
</td> | ||
<td align="center"> | ||
<a href="https://github.com/mnm-matin"> | ||
<img src="https://avatars.githubusercontent.com/u/45293386?v=4" width="100;" alt="mnm-matin"/> |
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.
scripts/add_extra_components.py
Outdated
|
||
"Split bidirectional links into two unidirectional links to include transmission losses." |
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.
Usually, we are using """
to write docstrings
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.
Thank you @Eric-Nitschke for this very important contribution.
I took the liberty to add some comments in order to cover this PR #1192 as well as PR #1215 and Issue #1213 at once. Feel free to discuss, if you think things can be done differently. Thanks again for the great work.
README.md
Outdated
<td align="center"> | ||
<a href="https://github.com/euronion"> | ||
<img src="https://avatars.githubusercontent.com/u/42553970?v=4" width="100;" alt="euronion"/> | ||
<br /> | ||
<sub><b>Euronion</b></sub> | ||
</a> | ||
</td> | ||
<td align="center"> | ||
<a href="https://github.com/Justus-coded"> | ||
<img src="https://avatars.githubusercontent.com/u/44394641?v=4" width="100;" alt="Justus-coded"/> | ||
<br /> | ||
<sub><b>Justus Ilemobayo</b></sub> | ||
</a> | ||
</td> | ||
<td align="center"> | ||
<a href="https://github.com/mnm-matin"> | ||
<img src="https://avatars.githubusercontent.com/u/45293386?v=4" width="100;" alt="mnm-matin"/> |
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.
Hello @Eric-Nitschke and @Eddy-JV! Great work on reviewing the PR. @Eric-Nitschke do you need any support in revisions? If they look heavy, could it make sense to breakdown the task? That is a precious contribution, and it would be great to have it merged into the model 🙂 |
Hey @ekatef, in general I think the revisions look quite doable and I'm planning on finishing them before the holidays! Major changes:
Minor changes:
Discussions for another time:
Did I miss anything? Tests:
|
Hey @Eric-Nitschke @ekatef . I think the PR is ready for merge once the above checklist provided by @Eric-Nitschke is done. I also replied to @Eric-Nitschke's comment regarding the config file. Thank you @Eric-Nitschke for the great work and looking forward for future contributions. |
Hello 😄 @Eric-Nitschke thanks a lot for all the responses and creating a check-list, thank you so much @Eddy-JV for the in-depth review and providing insights into the architecture 🙏🏽 I wonder what is the plan to proceed? @Eric-Nitschke I see that you have ticked the boxes but it appears that the changes have not been pushed to github. Do you experience any issues with git? [I have no ideas why but git is normally provides you with a lot of surprises when you start working in a repo] Happy to assist in resolving them if needed. Also, I see that there are some conflicts with the main. They result from come changes we introduced recently inside main and can be resolved relatively easily. Let me know if any hints on that would be helpful @Eddy-JV on the architectural level, I understand the idea is to polish the pipelines representation to a level when the model is truly realistic. Though, I have a feeling that the work needed may be considerable. @Eric-Nitschke do you see how to proceed with the advanced implementation proposed by @Eddy-JV? May be any break-down helpful? |
Major changes: - Fix bidirectional lossy links by splitting the into to links in _helpers.py - Add length based efficiency calculations in _helpers.py
Hey @ekatef, I had some issues with testing the changes I made since I wasn't at my usual setup and I didn't want to merge untestes changes into the pull request. However you can find the changes I made in a sperate branch here: https://github.com/Eric-Nitschke/pypsa-earth-bidirectional_lossy_links/tree/lossy_length_based |
Bug fixes: - modify the override component location provided in the Snakefile to work with the "copy-minimal" shadow directory on Windows - fix the identification of all forward links in scripts/solve_network.py
Hey @ekatef, hey @Eddy-JV, Aside from this, I'll remove the merge conflicts and then everything should be good to go! |
Minor changes: - move the transmission efficiency of the H2 pipelines to the one of the transmission grid, which was added with the last merge.
for more information, see https://pre-commit.ci
As I've mentioned during the discord call: |
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.
Hello @Eric-Nitschke @Eddy-JV, thank you for a fantastic collaborative work!
@Eric-Nitschke I see that you have implemented the massive improvements, and they look very good to me. Have added two rather minor comments comments which relate to the integration of the contribution into the codebase. Another point is CI which is currently complaining about /data/override_component_attrs
missed. Do you have any ideas how to fix that?
On the side of the domain-related functionality, @Eddy-JV could you please take a look? Given your expert knowledge on H2 systems, it would be perfect to have your review. My personal impression is that we are closing to finalise 🙂
Snakefile
Outdated
if os.name == "nt" | ||
else "/data/override_component_attrs" | ||
), | ||
# on Windows os.getcwd() is required because of the "copy-minimal" shadow directory |
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.
Just to be sure I get this point properly: is shadow directory the one which is created by the solver to store the temporary files? I wonder if this behaviour may also depend on a solver...
Regarding the files management, it has been found previously that os.getcwd( )
may lead to some troubles if pypsa-earth model is being used as snakemake submodule. In particular #1137 has fixed this replacing os.getcwd( )
by a custom directory path. May it be a way to go also in our case?
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.
In short:
The shadow directory is a Snakemake thing and not solver related.
With the "shadow" rule Snakemake creates an isolated temporary directory, in which it runs the corresponding rule (in this case "solve_network"). With shadow: "shallow"
, Snakemake symlinks all top level files and directory so that all relative file paths provided will still reach the right directories. However with shadow: "copy-minimal"
, the inputs of the rule are copied instead of symlinked. In my tests this led to Snakemake not having access to the files within the "data/override_component_attrs" folder. My guess is, that copy minimal only copies files directly, but not the content of directories. PR #790 is the reason, why "copy-minimal" is needed on Windows.
You can check the Snakemake documentation for more details, though it is not that extensive.
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.
However, I think I found the issue:
else "/data/override_component_attrs"
should be
else "data/override_component_attrs"
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.
Generally, itmight be nice to move away from os.getcwd()
to the changes in #1137. However, I am not confident in implementing these changes myself.
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.
@davide-f This definitely needs your attention as you already made PR #790. And @Eric-Nitschke if this approved by Davide, then probably the solution should be done for all Rules that use the solve_network.py script.
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.
os.getcwd() must be avoided as it does not track the use of submodules and can lead to issues.
Have you tried with directory(data/...)? [although the directory command should be optional for inputs].
Moreover, not sure if copy-minimal is strictly needed here; have you tested?
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.
@davide-f I've tried running it on Windows with "shallow" instead, which led to issues with admin rights similar to PR #790.
I've not tried directory(data/...) yet. Would that look something like this?
input:
overrides=(
directory("/data/override_component_attrs")
if os.name == "nt"
else "/data/override_component_attrs"
),
Removing the different handling (as done by @Eddy-JV) will most likely mean, that the code will not work properly on Windows since it will not find the proper files when running with "copy-minimal" (an issue that will not be solved by PR #1295).
However, I'm in favor of doing it this way, if it means that we'll merge this PR somewhat soon and create a new issue for the Windows issues.
config.default.yaml
Outdated
Link: [] # H2 pipeline | ||
Link: [H2 pipeline] # H2 pipeline |
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.
Just for better understanding: what is a particular idea of changing the default configuration parameters?
Agree that it may it be a good idea to make sure that the improvements introduced by this PR are being tested. It can be done by adding H2 pipeline
as extendable_carriers
:
pypsa-earth/test/config.sector.yaml
Lines 21 to 24 in ef525ee
electricity: | |
extendable_carriers: | |
Store: [] | |
Link: [] |
What do 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.
Yes testing was the main reason for adding it. I've moved it to test/config.sector.yaml.
Minor changes: - fix the input files for solve_network in the Snakefile for non-Windows operating systems. - revert the addition of "H2 Pipeline" to config.default.yaml and move it to the test/config.sector.yaml instead.
Minor changes: - fix test/config.sector.yaml by adding H2 Stores
The failed readthedocs relates to issue #1291. |
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.
@Eric-Nitschke Thank you once again for the great work. I only have few comments shown above. After that the PR can be merged without my review.
@davide-f please check the os.getcwd
comment.
@davide-f @ekatef I guess PR #1172 will have to be adapted to the new contraint function added here.
Snakefile
Outdated
if os.name == "nt" | ||
else "/data/override_component_attrs" | ||
), | ||
# on Windows os.getcwd() is required because of the "copy-minimal" shadow directory |
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.
@davide-f This definitely needs your attention as you already made PR #790. And @Eric-Nitschke if this approved by Davide, then probably the solution should be done for all Rules that use the solve_network.py script.
scripts/add_extra_components.py
Outdated
lossy_bidirectional_links(n, "H2 pipeline") | ||
|
||
# set the pipelines efficiency and the electricity required by the pipeline for compression | ||
set_length_based_efficiency(n, "H2 pipeline", " H2", config) |
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.
@Eric-Nitschke please add a variable like transmission_efficiency = snakemake.params.transmission_efficiency
this means cahnging this to: set_length_based_efficiency(n, "H2 pipeline", " H2", transmission_efficiency )
and in the snakefile please add to the rule add_extra_components:
params:
transmission_efficiency=config["sector"]["transmission_efficiency"],
in order to trigger the rule in case the efficiencies were changed in the config file by the user.
Minor changes: - add a transmission efficiency parameter to the Snakefile for add_extra_components and change the bidirectional lossy link implementation in _helpers.py to use the parameter rather than the full config as input - add the adjustment to the override component input for Windows in the Snakefile to all rules calling solve_network.py - revert the identification of backward links in solve_network.py to again accommodate the myopic naming scheme
for more information, see https://pre-commit.ci
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.
Hello!
I've added the comment about os.cwd; we must avoid that unfortunately.
Regarding the linopy implementation of the new constraint:
- when is this PR ready?
- does this contraint must be added all the time or optionally?
Ideally, we should ideally not delay further too much the merge of the linopy PR [expected basically in these days after long preparation...]
Snakefile
Outdated
if os.name == "nt" | ||
else "/data/override_component_attrs" | ||
), | ||
# on Windows os.getcwd() is required because of the "copy-minimal" shadow directory |
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.
os.getcwd() must be avoided as it does not track the use of submodules and can lead to issues.
Have you tried with directory(data/...)? [although the directory command should be optional for inputs].
Moreover, not sure if copy-minimal is strictly needed here; have you tested?
…ix: Fix admin needs for Windows pypsa-meets-earth#1295
@davide-f technically the constraint is only required, if at least one lossy bidirectional component is used. Adapting the constraint to PR #1172 should be somewhat easy, as the PyPSA-Eur implementation already used the newer PyPSA version including linopy.
to
which is the section I changed to make it work with linopf. |
@Eric-Nitschke Thank you for swiftly addressing my comments and @davide-f Thanks for checking this up. Now that this PR is mature enough to be merged and very much needed ASAP for the Linopy adaptation, I took the liberty to change this:
back to
from the current PR and address it seperately in PR #1295 or another PR. Awesome work @Eric-Nitschke , I will merge now. |
@Eric-Nitschke, @Eddy-JV, @davide-f fantastic work!! 😄 Great to see the contribution completed and merged 🎉 @Eric-Nitschke if you wish to go ahead with adapting the constraint for linopy, feel free to open a PR on that 🙂 |
@Eric-Nitschke just flagging that the constraint update has been implemented #1276 as it's crucial for the model update. Apologies if I have been misleading and thanks a lot for the major contribution in resolving the folder management issue for windows! |
Summary:
Changes:
Reason:
Checklist
envs/environment.yaml
anddoc/requirements.txt
.config.default.yaml
andconfig.tutorial.yaml
.test/
(note tests are changing the config.tutorial.yaml)doc/configtables/*.csv
and line references are adjusted indoc/configuration.rst
anddoc/tutorial.rst
.doc/release_notes.rst
is amended in the format of previous release notes, including reference to the requested PR.