-
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
Bugfix for csp buses: add buses one for generator #1076
Conversation
@GbotemiB I've found a small fix with csp. The error is attached. FYI @ekatef |
Thanks a lot @davide-f! @GbotemiB I think it would be great to summarise in plain language the reason why the re-implementation has been needed. It feels for me that we could have missed some important details, when defining a problem for CSP implementation. Could you please look into that in some more details to make sure that we are lean from it? One more point which we may want to check is definition of |
Hello @ekatef, I have looked into the issue you mentioned about CSP having This is majorly due to the fact that I also confirm that the implementation in pypsa-eur is the same. We can check pypsa-eur, if they have similar issues, or they fixed it already. |
Hello @GbotemiB, thank you for looking into both points. Totally agree and thanks for the explanation. May it be worth to open an issue on |
Hello! @davide-f thanks for taking care about this part. The implementation looks great. That is a bug fix, and it would be good to have it included it as fast as we can |
Update: this fix can lead to some interactions with the merge PR, as the latter also includes the fix for a similar problem. It makes absolute sense to coordinate merging both PRs |
nope :D feel free to |
Hi, will this issue be resolved soon? I am trying to run pypsa-earth on Brazil but hit this issue three weeks ago and again today: KeyError: "['BR6 0 csp'] not in index" |
@cshearer1977, can you try to drop |
@cshearer1977 thank you for the reminder. The PR was put on hold to avoid conflicts with other work in the repository (in particular, PR #1086). But you are completely right that the bug-fixing is needed. @davide-f have added a small review comments. Otherwise, looks great in my opinion 😄 Let me know please if you need any support with merging it ;) |
@GbotemiB: Thanks for the suggestion. I deleted "csp" from the renewable carriers in the config file but got this error message:
Is the final list of power plants available as an output somewhere? Maybe I could delete the CSP plants and input the remaining plants into the custom power plant csv file. |
I think you will have to delete the resources folder, then rerun the model. The reason is that you are still running, with the |
Thanks. Before rerunning the model I did |
It should delete the content, but if it doesn't, you can just delete the resources folder. |
Many thanks everyone! @cshearer1977 does this pr fix most of your issues? |
Hi @davide-f, thank you for asking. On August 13 and again on September 2, I ran PyPSA-earth for Brazil (country code "BR") with just one modification: changing retrieve cost data to False with some higher values for coal in the costs.csv file. Both times the program failed and gave me this error message: I reported the issue here and @GbotemiB suggested I drop csp from renewable carriers, which I took to mean deleting csp from the list of renewable carriers in the config file. I did snakemake clean and reran the program without csp and got this error message: Given your recent commit I tried to clone the repo again today and retry (without deleting csp) and got the same error as before: Here is more of the error message:
Any suggestions? Also, I am usually near the final stages when I hit the error, so is there a way I can try some fixes without completely rerunning the program for hours? Thanks! |
That is interesting and you can discover the reason for that. pypsa-earth/scripts/add_extra_components.py Line 271 in 1bd7f84
to match the wildcards in your config, you can debug the script using vscode or spyder. That runs the python script without running the whole workflow and you can identify why that appears. Do you use alternative_clustering? This issue may be related to that too but not sure. |
@davide-f @GbotemiB is this PR ready to be merged? Happy to support if needed 🙂 |
From my end, it is ready to be merged. |
Merging to keep the model fully functional before the merge |
Closes # (if applicable).
Changes proposed in this Pull Request
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.