-
Notifications
You must be signed in to change notification settings - Fork 210
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
Custom ppl opts #739
Custom ppl opts #739
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.
Nice PR! 💯 Some minor changes are required.
Could you also adjust the configuration.rst accordingly?
config.default.yaml
Outdated
@@ -142,7 +142,7 @@ electricity: | |||
Link: [] # H2 pipeline | |||
|
|||
powerplants_filter: (DateOut >= 2022 or DateOut != DateOut) | |||
custom_powerplants: false | |||
custom_powerplants: false # "false" - no custom powerplants, "merge" uses both open-source and custom powerplants, "replace" uses only custom powerplants |
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 you change the comment to wherever possible:
# "false" use only powerplantmatching (ppm) data, "merge" combines ppm and custom powerplants, "replace" use only custom powerplants
scripts/build_powerplants.py
Outdated
# if isinstance(custom_ppl_query, str): | ||
# add_ppls.query(custom_ppl_query, inplace=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.
I know pypsa-eur is using line 247 and 248. Can you figure out its purpose?
I think your merge and replace option could make lines 247 and 248 not necessary. Maybe we can remove the comment?
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 have checked the code and it seems that it is left to enable some query to custom powerplants being added. We can leave them as commented or we can remove them.
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.
But it is better to remove them, since if enabled it might contradict with the proposed PR.
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.
Looks great. I just had to change the release note position
Changes proposed in this Pull Request
Here I propose an extension of functionality of custom powerplants addition. Currently, there are only two options: 1) Use open-source powerplants with
custom_powerplants: false
, and 2) Attach custom powerplants fromcustom_powerplants.csv
to open-source data and use both withcustom_powerplants: true
. There is no option to select only custom powerplants. This functionality is very useful. Sometimes you can have an accurate custom powerplants information and you would like to replace retrieved open-source plants with custom data. On the other situations, you might need to use both open-source and custom powerplants.Therefore, I propose to extend the functionality to three options:
false
- no custom power plants are used;merge
- both open-source and custom powerplants fromcustom_powerplants.csv
are merged and used;replace
- only custom powerplants fromcustom_powerplants.csv
are used.Checklist
config.default.yaml
andconfig.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.