-
Notifications
You must be signed in to change notification settings - Fork 187
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
Update PyPSA version #1065
Update PyPSA version #1065
Conversation
This reverts commit 7700759.
78cab98
to
8f4f7ea
Compare
A testing note the PR and in particular specifying the aggregation strategy for mainINFO:__main__:Objective function: 4567465309.0
INFO:__main__:Objective constant: 2127041137.1933956 update-pypsa_versionINFO:__main__:Objective function: 4185403898.0
INFO:__main__:Objective constant: 2126804937.1863549 update-pypsa_version + do not include v_nom into the aggregation strategyINFO:__main__:Objective function: 5112254879.0
INFO:__main__:Objective constant: 2126771900.9923582 |
An additional comment: update of PyPSA requites some adjustments in fetching isolated networks (merge_into_network() function). It may be worth to track it as an additional issue. |
The reason of the weird behaviour is specifying Resuts of updated objective testmain
update-pypsa_version
That still means ~1% difference for the objective function. It seems that the difference is linked with |
Points to be tracked:
|
Results of the compatibility testing for updating PyPSA version: currently building the environment on mac seems to lead to some weird problem still in a dry run Pinging all the versions in |
I think the mac issue is related to some update that is needed with conda. I have read a few issues related to this. We can test to see if it works. |
Thanks for sharing! Do you have any ideas for fixing? I'm currently testing it, and any hints would be tremendously helpful |
Regarding the overall approach, the proposal is totally fine! I confirm that this issue with mac is not pypsa-version related. An interesting option is to check the latest conda envs that worked and see if there are differences.
There are dissimilarities in some internal conda packages that may be explored. |
Super, thanks for the confirmation for the general approach. Then, I'll leave out the analysis of the reasons of the 1% difference, and will finalise the work by cleaning-up and merging both PRs to have a single one. Regarding the environment issue, I confirm that the problem seems to be the same as in CI. When trying to run the workflow with an updated environment, I get |
Update on the environment testing: have applied version restrictions to |
A comment regarding merging the isolated networks: PyPSA update leads to a weird issue in In particular, the troubles are triggered by aggregating of loads and storage units with this line: pypsa-earth/scripts/simplify_network.py Line 861 in 1bd7f84
The issue is linked with The full log looks like that: ERROR:_helpers:An error happened in module '~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/indexes/base.py', function 'get_loc': 'p'
Traceback (most recent call last):
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/indexes/base.py", line 3805, in get_loc
return self._engine.get_loc(casted_key)
File "index.pyx", line 167, in pandas._libs.index.IndexEngine.get_loc
File "index.pyx", line 196, in pandas._libs.index.IndexEngine.get_loc
File "pandas/_libs/hashtable_class_helper.pxi", line 7081, in pandas._libs.hashtable.PyObjectHashTable.get_item
File "pandas/_libs/hashtable_class_helper.pxi", line 7089, in pandas._libs.hashtable.PyObjectHashTable.get_item
KeyError: 'p'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/runpy.py", line 196, in _run_module_as_main
return _run_code(code, main_globals, None,
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/runpy.py", line 86, in _run_code
exec(code, run_globals)
File "~/.vscode/extensions/ms-python.debugpy-2024.8.0-darwin-arm64/bundled/libs/debugpy/adapter/../../debugpy/launcher/../../debugpy/__main__.py", line 39, in <module>
cli.main()
File "~/.vscode/extensions/ms-python.debugpy-2024.8.0-darwin-arm64/bundled/libs/debugpy/adapter/../../debugpy/launcher/../../debugpy/../debugpy/server/cli.py", line 430, in main
run()
File "~/.vscode/extensions/ms-python.debugpy-2024.8.0-darwin-arm64/bundled/libs/debugpy/adapter/../../debugpy/launcher/../../debugpy/../debugpy/server/cli.py", line 284, in run_file
runpy.run_path(target, run_name="__main__")
File "~/.vscode/extensions/ms-python.debugpy-2024.8.0-darwin-arm64/bundled/libs/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py", line 321, in run_path
return _run_module_code(code, init_globals, run_name,
File "~/.vscode/extensions/ms-python.debugpy-2024.8.0-darwin-arm64/bundled/libs/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py", line 135, in _run_module_code
_run_code(code, mod_globals, init_globals,
File "~/.vscode/extensions/ms-python.debugpy-2024.8.0-darwin-arm64/bundled/libs/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_runpy.py", line 124, in _run_code
exec(code, run_globals)
File "~/Documents/_github_/pypsa-earth/scripts/simplify_network.py", line 1153, in <module>
n, fetched_nodes_map = merge_into_network(
File "~/Documents/_github_/pypsa-earth/scripts/simplify_network.py", line 862, in merge_into_network
clustering = get_clustering_from_busmap(
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pypsa/clustering/spatial.py", line 519, in get_clustering_from_busmap
new_df, new_pnl = aggregateoneport(
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pypsa/clustering/spatial.py", line 266, in aggregateoneport
data = n.get_switchable_as_dense(c, attr)
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pypsa/descriptors.py", line 124, in get_switchable_as_dense
vals = np.repeat([df.loc[fixed_i, attr].values], len(snapshots), axis=0)
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/indexing.py", line 1184, in __getitem__
return self._getitem_tuple(key)
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/indexing.py", line 1368, in _getitem_tuple
return self._getitem_lowerdim(tup)
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/indexing.py", line 1065, in _getitem_lowerdim
section = self._getitem_axis(key, axis=i)
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/indexing.py", line 1431, in _getitem_axis
return self._get_label(key, axis=axis)
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/indexing.py", line 1381, in _get_label
return self.obj.xs(label, axis=axis)
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/generic.py", line 4287, in xs
return self[key]
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/frame.py", line 4102, in __getitem__
indexer = self.columns.get_loc(key)
File "~/opt/miniconda3/envs/pypsa-earth-upg5/lib/python3.10/site-packages/pandas/core/indexes/base.py", line 3812, in get_loc
raise KeyError(key) from err
KeyError: 'p' |
I think I have come across this error before. Although, I cant clearly remember how I solved it. |
Hey @GbotemiB, thanks for sharing the experience! In this case, the problem is localised and reproducible. Probably, we'll be able to discover also the reason behind the previous troubles, when looking into it. But agree that the issue sound quite weird, and I'm thinking that it may be better to track it by a dedicated 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.
Hello @ekatef :D
Just recommenting here to check the status of this PR [and retrigger the CI].
Added a minor comment but functionality is there :)
It would be good to update to main to check and aim to merge this and linopy transition.
They could also be merged the PR and tackled as one?!?!
I'm unsure the update of linopy works without the revision of the functions of extra functionalities
Please comment if you need support
envs/environment.yaml
Outdated
@@ -12,7 +12,7 @@ dependencies: | |||
- pip | |||
- mamba # esp for windows build | |||
|
|||
- pypsa>=0.24, <0.25 | |||
- pypsa>=0.25, <0.28 |
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 increase the version further?
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.
Yeas, I have been thinking about testing for <0.29
. The main concern is that v0.29.0
drops n.lopf( )
which I'm not quite sure is really a great idea. What is your feeling about that?
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.
Interesting; probably worth updating till <0.29 maybe?
=0.29 can be tested in the linopy PR with linopy; that PR should fix that too.
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.
With 0.28 testing successful :)
scripts/build_osm_network.py
Outdated
# Keep only a predefined set of columns, as otherwise conflicts are possible | ||
# e.g. the columns which names starts with "bus" are mixed up with | ||
# the third-bus specification when executing additional_linkports() | ||
lines_cols_standard = [ |
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.
This could also be a global variable at the beginning with capitalized characters.
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.
Done. Have I got your idea?
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.
Done. Have I got your idea?
Great :D basically yes :) as a little comment, would you mind placing it around line 25-26ish?
Having it as a global variable could help as it may be loaded by other scripts if it will ever be necessary.
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.
Makes sense :) Done!
for more information, see https://pre-commit.ci
@davide-f thanks a lot for looking into that! 😄 The idea to split both tasks was to disentangle the issue with non-documented changes in the network structure. As the next step, agree that the linopy PR should be merged into this one, and probably also linopy PR to the cross-sectoral model [not sure what would be the most elegant approach]. |
As a technical comment, the CI currently fails for mac with |
Let's rerun it and see what happens... that is a new as it worked before but it can be mamba related. Update: it persists; I'm fine with updating it. Would you like to tackle it here? |
I'd be more comfortable to see CI running before merging the PR. But we can probably tackle it in #1128 together with others dependencies fixes. What do you think? |
Have re-tested the PR with PyPSA v0.28.0 and it works. The only trouble relates to some weird error in merging isolated networks. It has been seen before, and feels like something related to the environment. So, I'd focus on updating and stabilising the environment for now (which includes this PR and enabling linopy), and work on this fix for this particular problem after. Otherwise, the testing seems to be fine with the difference in the objective function less 1%. |
Closing in favour #1169 |
Disentangles update of PyPSA version in #796 from changes needed to implement
linopy
into model construction.Covers #787 and includes adaptation of the changed PyPSA implications for
n.buses
andn.lines
structure to our current data structures.As a note: the strategy towards processing "non-standard" columns has changed in the recent PyPSA versions. Non-standard means names which are not included into columns of components attributes (see lines attributes, for example).
Changes proposed in this Pull Request
The PR follows the approach of PyPSA-Eur, but introduces additional functionality to define custom aggregation strategies in a flexible way.
TODO It would be nice to have a more formalised definition of the components data structures in PyPSA. Currently, the custom columns are implicitly allowed, but lead to troubles during clustering.
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.