-
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
Merged
Merged
Changes from 29 commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
ea57cea
Lossy Bidirectional Links
Eric-Nitschke 9acbfe4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] a1f91e4
Update release notes
Eric-Nitschke 31c7973
Merge remote-tracking branch 'origin/main'
Eric-Nitschke a0c0a05
Spelling fix
Eric-Nitschke a2150d6
Merge branch 'main' of github.com:pypsa-meets-earth/pypsa-earth
Eric-Nitschke 1a98c9f
docs(contributor): contrib-readme-action has updated readme
github-actions[bot] ac90aec
Revert "docs(contributor): contrib-readme-action has updated readme"
Eric-Nitschke a3bd91d
docs(contributor): contrib-readme-action has updated readme
github-actions[bot] 80aee27
Fix ci (#1210)
davide-f 45fdc2b
Merge branch 'main' of github.com:Eric-Nitschke/pypsa-earth-bidirecti…
Eric-Nitschke 77f557f
git@github.com:pypsa-meets-earth/pypsa-earth.git
davide-f e38b221
Fix bidirectional lossy links
Eric-Nitschke fefe70b
Constraint implementation bug fixes
Eric-Nitschke 4738a3f
Revert "Merge branch 'main' of github.com:Eric-Nitschke/pypsa-earth-b…
Eric-Nitschke e108be5
Merge branch 'lossy_length_based'
Eric-Nitschke 9f55920
Merge remote-tracking branch 'upstream/main'
Eric-Nitschke 2ac8db4
Unify transmission efficiency
Eric-Nitschke 02fc071
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 9d14ac2
Spelling fix
Eric-Nitschke f6aa253
Merge branch 'main' of github.com:Eric-Nitschke/pypsa-earth-bidirecti…
Eric-Nitschke ca1db0a
Release notes update
Eric-Nitschke 7c87a48
Bugfix Snakefile for non-Windows operating systems
Eric-Nitschke 4092176
Merge branch 'main' into main
Eric-Nitschke fcb9911
Bugfix test config
Eric-Nitschke 007e427
Merge branch 'main' of github.com:Eric-Nitschke/pypsa-earth-bidirecti…
Eric-Nitschke b1ef404
Merge branch 'main' into main
Eric-Nitschke 090eeb9
Final adjustments for bidirectional lossy links
Eric-Nitschke d84ce66
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] eaba5cf
Update Snakefile: Remove os.getcwd from this PR. To be solved in Bugf…
Eddy-JV File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 replacingos.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 withshadow: "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 beelse "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?
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.