-
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
Finalize duplication of retrieve_data #1249
Conversation
for more information, see https://pre-commit.ci
77a829f
to
6022316
Compare
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 @davide-f, looks like a very nice improvement!
My major concern is that the PR also contains some improvements which relate to improvements for alternative clustering. It feels like there may be some connections with other issues and fixes, but the context is not quite clear from the PR. Given the fact that alternative clustering seems to be one of the major points to be tackled, I'd suggest to keep in this PR only cost-related improvements and open another dedicated PR for the improvements on alternative clustering. What do you think?
The costs-related part looks great and basically ready to merge, unless there are some comments from @hazemakhalek @Eddy-JV and @energyLS
@davide-f thanks a lot for introducing the changes! Totally agree that tackling the hydro improvements deserves a dedicated issue. Thanks for taking care about that 🙂 I think this PR is ready to merge if there is no objections from the experts on the sector-coupled part 🚀 Great work! 😄 |
@energyLS doublecheck |
5dbbef3
to
c10d1c8
Compare
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 very good! 😄
I remember that the PR combines changes related both to file management for costs and improvements for hydro calculations. To help disentangle it, may it be a good idea to add a duplicated release note on hydro along with the existing one?
Have checked the objective, and it appears that the only significant change of ~8% relates to the second test case (the one with Also, there is ~4% change for one of the Monte-Carlo tests (the one with very low objective function), but I remember that we had it previously (looking on this comment) and it's normal. So, the objective function didn't change for the test cases with Voronoi clustering and slightly increased for |
6de65c1
to
c406a1d
Compare
Closes #1073 and #1074
Changes proposed in this Pull Request
This PR builds on top of #1120 to avoid duplication of retrieve_data function, bump technology data version and drop cost_dir option.
Note: dropping cost_dir is in line with pypsa-eur and we also had a TODO on that, but before merging this PR, there is the need for a check by @hazemakhalek , @Eddy-JV or @energyLS given its use in the -sec model.
Moreover, bumping the latest version of the default technology data to the latest will change values of the CI
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.