-
Notifications
You must be signed in to change notification settings - Fork 204
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
Linopy integration for Power and Sector-Coupled Modelling #1172
base: main
Are you sure you want to change the base?
Conversation
* Add a zenodo link to natura.tiff * Update environment * Revise structure definition for lines * Remove get_aggregation_strategies * Fix typo aggregation_strategies * Replace aggregategenerators with aggregateoneport * Add aggregation strategies as a parameter * Re-define aggregation strategies * Update aggregation strategies * Update aggregation strategies for lines * Update aggregation strategies for buses * Fix typo * Put aggregation strategies into a variable * Parametrize the aggregation strategies * Refactor update of the aggregation strategies * Clean-up the code * Revert "Add a zenodo link to natura.tiff" This reverts commit 7700759. * Define an explicit clustering strategy for v_nom * Add a release note * Get glpk back * Specify v_nom for buses explicitly * Revert "Specify v_nom for buses explicitly" This reverts commit 20192e6. * Add a version restriction to the environment specification * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Adjust naming * Move the variable definition * Move the variable * Upgrade PyPSA version --------- Co-authored-by: Davide Fioriti <fioritidavidesubs@gmail.com> Co-authored-by: Davide Fioriti <67809479+davide-f@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Add a zenodo link to natura.tiff * Update environment * Revise structure definition for lines * Remove get_aggregation_strategies * Fix typo aggregation_strategies * Replace aggregategenerators with aggregateoneport * Add aggregation strategies as a parameter * Re-define aggregation strategies * Update aggregation strategies * Update aggregation strategies for lines * Update aggregation strategies for buses * Fix typo * Put aggregation strategies into a variable * Parametrize the aggregation strategies * Refactor update of the aggregation strategies * Clean-up the code * Revert "Add a zenodo link to natura.tiff" This reverts commit 7700759. * Define an explicit clustering strategy for v_nom * Add a release note * Get glpk back * Specify v_nom for buses explicitly * Revert "Specify v_nom for buses explicitly" This reverts commit 20192e6. * Add a version restriction to the environment specification * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Adjust naming * Move the variable definition * Move the variable * Upgrade PyPSA version * Update docstring * Fix imports duplication * Update imports * Update the carrier-capacity constraint * Add docstring * Update the equity constraint * Add docstring * Update BAU constraint * Update SAFE constraint * Add docstring * Update operational reserve margin constraint * Add docstring * Add an new argument to the RM constraint * Update the update of capacity constraints * Update adding an operational reserve margin constraint * Update docstring * Update battery constraint * Add docstring * Update a constraint related to a RES share * Fix usage of add_ERS_constraints * Update solving script * Update a solving run * Fix typos --------- Co-authored-by: Davide Fioriti <fioritidavidesubs@gmail.com> Co-authored-by: Davide Fioriti <67809479+davide-f@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@GbotemiB As discussed, we can use a branch of this PR ( |
Thanks @ekatef. I can start working on the linopy PR for the sec aspect. |
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.
Great :)
Very minor things, basically ready to merge :)
scripts/solve_network.py
Outdated
nodes = n.buses.index[n.buses.carrier == "battery"] | ||
if nodes.empty or ("Link", "p_nom") not in n.variables.index: | ||
# TODO Check if the second part of the condition can make sense |
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.
Does this still apply?
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.
My understanding is that this second part addresses some special case when the network doesn't contain any batteries, despite of the fact that there are nodes with battery
carrier. Not sure when exactly can it be the case and just leaving it as is for now, as it seems not to relate to interfacing with linopy. Does it make sense for you or am I missing something?
scripts/solve_network.py
Outdated
lgrouper = n.loads.bus.map(n.buses.country) | ||
# TODO drop load |
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.
Is this for a later stage?
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, my feeling is that the enclosing add_RES_constraints( )
will need some additional revisions which should go beyond this PR.
In our current upstream implementation, the function throws a warning stating that it's only a preliminary implementation. I think, it may be a good idea to open an issue to track further improvements
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 the updates version of add_RES_constraints( )
, this TODO is not needed anymore
scripts/solve_network.py
Outdated
# Generators | ||
# TODO restore grouping by countries un-commenting calls of groupby() |
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.
Does this not work? for 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.
Yeah, linopy.groupby( )
has been not in the best shape when I have been debugging this part. So, I have left it out for future improvements of add_RES_constraints( )
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.
Maybe we should add a warning&issue then?
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.
Have fixed the functionality. It has been more straight-forward ;)
scripts/solve_network.py
Outdated
n.lines.loc[n.lines.index.str.contains("new"), "s_nom_min"] = ( | ||
snakemake.params.augmented_line_connection.get("min_expansion") | ||
) | ||
# TODO Double-check handling the augmented 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.
Why comment this? Does it trigger an issue?
It seems look it generates issues, 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.
A great catch! That is just an old comment needed to debug the main functionality. Now it must be lifted
Testing it
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.
Have get this part back, and testing worked well
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.
can we drop this then?
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* Add a zenodo link to natura.tiff * Update environment * Revise structure definition for lines * Remove get_aggregation_strategies * Fix typo aggregation_strategies * Replace aggregategenerators with aggregateoneport * Add aggregation strategies as a parameter * Re-define aggregation strategies * Update aggregation strategies * Update aggregation strategies for lines * Update aggregation strategies for buses * Fix typo * Put aggregation strategies into a variable * Parametrize the aggregation strategies * Refactor update of the aggregation strategies * Clean-up the code * Revert "Add a zenodo link to natura.tiff" This reverts commit 7700759. * Define an explicit clustering strategy for v_nom * Add a release note * Get glpk back * Specify v_nom for buses explicitly * Revert "Specify v_nom for buses explicitly" This reverts commit 20192e6. * Add a version restriction to the environment specification * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Adjust naming * Move the variable definition * Move the variable * Upgrade PyPSA version --------- Co-authored-by: Davide Fioriti <fioritidavidesubs@gmail.com> Co-authored-by: Davide Fioriti <67809479+davide-f@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Add a zenodo link to natura.tiff * Update environment * Revise structure definition for lines * Remove get_aggregation_strategies * Fix typo aggregation_strategies * Replace aggregategenerators with aggregateoneport * Add aggregation strategies as a parameter * Re-define aggregation strategies * Update aggregation strategies * Update aggregation strategies for lines * Update aggregation strategies for buses * Fix typo * Put aggregation strategies into a variable * Parametrize the aggregation strategies * Refactor update of the aggregation strategies * Clean-up the code * Revert "Add a zenodo link to natura.tiff" This reverts commit 7700759. * Define an explicit clustering strategy for v_nom * Add a release note * Get glpk back * Specify v_nom for buses explicitly * Revert "Specify v_nom for buses explicitly" This reverts commit 20192e6. * Add a version restriction to the environment specification * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Adjust naming * Move the variable definition * Move the variable * Upgrade PyPSA version * Update docstring * Fix imports duplication * Update imports * Update the carrier-capacity constraint * Add docstring * Update the equity constraint * Add docstring * Update BAU constraint * Update SAFE constraint * Add docstring * Update operational reserve margin constraint * Add docstring * Add an new argument to the RM constraint * Update the update of capacity constraints * Update adding an operational reserve margin constraint * Update docstring * Update battery constraint * Add docstring * Update a constraint related to a RES share * Fix usage of add_ERS_constraints * Update solving script * Update a solving run * Fix typos --------- Co-authored-by: Davide Fioriti <fioritidavidesubs@gmail.com> Co-authored-by: Davide Fioriti <67809479+davide-f@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
7ce0894
to
8172112
Compare
Co-authored-by: Davide Fioriti <67809479+davide-f@users.noreply.github.com>
@davide-f thanks for the review! 😄 I confirm that me and Emmanuel have checked that the PR doesn't change the objective function. Though, the may be some pitfalls with testing extra-functionality functions. Have to check it. Currently, CI is failing as it's using pinned environments which must be re-build using the updated |
… into pypsa_linopy_update
6efea61
to
75b60ca
Compare
* Update pinned environment files for all platforms --------- Co-authored-by: ekatef <30229437+ekatef@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.
Added few comments related to the reserve constraints, hoping to help
lhs = merge( | ||
dispatch * 1, | ||
reserve * 1, | ||
) | ||
|
||
if not ext_i.empty: |
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 pypsa-eur, there is no need for this if case, though I don't think is the reason of the issue
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.
True. Probably p_max_pu[ext_i]
is safe for empty ext_i
. Have added a TODO to revise this part
scripts/solve_network.py
Outdated
.loc[vres_i.intersection(ext_i)] | ||
.rename({"Generator-ext": "Generator"}) | ||
) | ||
lhs = merge( |
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 see you have made extensive use of the merge function.
I'm dropping a comment hoping it may help.
The documentation of merge informs that when a list of expressions is merged, it assumes they share the same index basically: in case of a list, the first index overrides the others.
https://linopy.readthedocs.io/en/latest/generated/linopy.expressions.merge.html
Not sure if it may have unintended consequences.
In pypsa-eur, I've seen a lower usage of merge but it may not be the cause of this issue
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.
Agree and revised, except for add_CCL_constraints
where it's not straght-forward (the current design implies forming an expression as a list, while pypsa-eur implementation has changed significantly)
scripts/solve_network.py
Outdated
) | ||
lhs = merge( | ||
lhs, | ||
(renewable_capacity_variables * (-EPSILON_VRES * capacity_factor)).sum( |
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.
if we rename p_nom_vres the renewable capacity variables, maybe we shorten a bit the text, but minor 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.
In pypsa-eur, they also include a xr.DataArray(capacity_factor), not sure if it has implications into the calculations
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.
Agree, aligned with pypsa-eur implementation
Update: re-testing of the objective function is working well in some testing cases, but leads to the differences in others. In particular, the tutorial-based test is not working properly. I'm able to reproduce the issue locally, and it appears to be linked with the changes introduced before Basically, the value of the objective function does not depend on the solving script but on the procedure before: solving the network prepared by A similar issue has been observed before for power-only model, and has been resolved by setting |
Many thanks for investigating. What is your insight on this issue and status of the PR therefore? can we track what has been merged that may have led to the issue? |
Let's remind to change the dependencies also in the file doc/requirements.txt. |
I've been testing the model and to me, it seems like the change in the config.tutorial.yaml are not due to changes in solve_network but something else. The checks on the network seem to suggest that the input networks match but something changes for some reason. At deeper analysis, it seems like the onwind (and marginally offwind) are different. Update: the onwind p_max_pu is expected to be the same after elec.nc [add_electricity]; probably there is something going on in the clustering steps that is different |
@davide-f many thanks for testing (and providing the fantastic infrastructure for inter-comparing the networks!). The issues indeed are quite weird and it has been precious to have your independent workflow to be sure that they are reproducible. Writing down the results of the testing below to possibly improve further both implementations 🙂 It has been found that the network structure and parameters are exactly the same for both implementations. The reason of the differences in the objective function are linked with the difference in In the older version of PyPSA, a default strategy have has been used for Points of action @davide-f great that we have been able to discover the reason, as it appears to be a potentially major issue also for the current implementation 🎉 🎉 🎉 Thank you so much for that. I think it's a great result, and we are very close to both finalise this PR and make the current implementation really stable 😄 |
It appears, the trick was to use an explicit definition of an aggregation strategy as The full testing results looks like follows: mainINFO:main:Objective function: 2512675455.0 this PRINFO:main:Objective function: 2512675751.0 That gives the following differences: config.custom.yaml1.178027e-07 config.landlock.yaml2.332123e-05 config.sector.yaml4.609890e-08 config.test_myopic.yaml6.200382e-05 For Monte-Carlo test, the differences may be very high ( |
@davide-f I think we have done it 😄 |
Closes #494
(and hopefully #968)
Changes proposed in this Pull Request
Integrated changes needed to update PyPSA version and use linopy.
To have a better overview, the PR has been implemented in three steps:
linopy
into power part of the model (follows Linopy transition #796, the diffs can be checked in this local PR)linopy
into cross-sectoral part of the model (implemented currently as a dedicated PR to the Sec- repo)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.