pypsa-earth icon indicating copy to clipboard operation
pypsa-earth copied to clipboard

Bugfix for csp buses: add buses one for generator

Open davide-f opened this issue 1 year ago • 9 comments

Closes # (if applicable).

Changes proposed in this Pull Request

Checklist

  • [x] I consent to the release of this PR's code under the AGPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
  • [ ] I tested my contribution locally and it seems to work fine.
  • [ ] Code and workflow changes are sufficiently documented.
  • [ ] Newly introduced dependencies are added to envs/environment.yaml and doc/requirements.txt.
  • [ ] Changes in configuration options are added in all of config.default.yaml and config.tutorial.yaml.
  • [ ] Add a test config or line additions to test/ (note tests are changing the config.tutorial.yaml)
  • [ ] Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
  • [x] A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

davide-f avatar Aug 07 '24 18:08 davide-f

@GbotemiB I've found a small fix with csp. The error is attached. ERROR_helpersAn error happened in m.txt Since you worked on it, it would be great to have your opinion.

FYI @ekatef

davide-f avatar Aug 07 '24 18:08 davide-f

Hi @davide-f, Thanks alot for catching this. Looks like the original implementation was not querying the index properly which is a reason for the error.

Looks good to me.

@ekatef, what are you thoughts?

GbotemiB avatar Aug 08 '24 15:08 GbotemiB

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 v_nom for CSP buses. I have noticed that in some cases v_nom is 1 kV for CSP buses in the simplified network and further. Would you mind to check it and submit a fix, if needed? My feeling is that it may be linked with the same problem you addressed when fixing the negative value problem

ekatef avatar Aug 11 '24 22:08 ekatef

@ekatef I think a simple explanation is that the original approach creates CSP buses without considering the generators, while the fix from @davide, fetches bus index from the generators which is then used to create buses for CSP.

I will look into the v_nom irregularity with CSP buses.

GbotemiB avatar Aug 12 '24 14:08 GbotemiB

Hello @ekatef, I have looked into the issue you mentioned about CSP having 1kV for v_nom. This seems to be the case for every renewable tech carrier in attach_stores except for AC.

This is majorly due to the fact that v_nom is not set. image

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.

GbotemiB avatar Aug 12 '24 18:08 GbotemiB

Hello @ekatef, I have looked into the issue you mentioned about CSP having 1kV for v_nom. This seems to be the case for every renewable tech carrier in attach_stores except for AC.

This is majorly due to the fact that v_nom is not set. image

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 v_nom values for the renewable generators? The effect of the objective result has been quite impressive 😄

ekatef avatar Aug 13 '24 22:08 ekatef

Hello! @davide-f thanks for taking care about this part. The implementation looks great. Do you have any objections if we'll merge this PR? 🙂

That is a bug fix, and it would be good to have it included it as fast as we can

ekatef avatar Aug 14 '24 08:08 ekatef

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

ekatef avatar Aug 14 '24 08:08 ekatef

Hello! @davide-f thanks for taking care about this part. The implementation looks great. Do you have any objections if we'll merge this PR? 🙂

That is a bug fix, and it would be good to have it included it as fast as we can

nope :D feel free to

davide-f avatar Aug 14 '24 09:08 davide-f

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 avatar Sep 03 '24 04:09 cshearer1977

@cshearer1977, can you try to drop csp from renewable_carriers in your config file? This should work as a temporary fix.

GbotemiB avatar Sep 03 '24 07:09 GbotemiB

@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 ;)

ekatef avatar Sep 03 '24 13:09 ekatef

@cshearer1977, can you try to drop csp from renewable_carriers in your config file? This should work as a temporary fix.

@GbotemiB: Thanks for the suggestion. I deleted "csp" from the renewable carriers in the config file but got this error message:

KeyError: "None of [Index(['BR0 0 csp', 'BR0 1 csp', 'BR0 2 csp', 'BR0 3 csp', 'BR0 4 csp',\n 'BR0 5 csp', 'BR0 6 csp', 'BR0 7 csp', 'BR0 8 csp', 'BR0 9 csp',\n 'BR1 0 csp', 'BR2 0 csp', 'BR3 0 csp', 'BR4 0 csp', 'BR5 0 csp',\n 'BR6 0 csp'],\n dtype='object', name='Bus')] are in the [index]"

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.

cshearer1977 avatar Sep 03 '24 19:09 cshearer1977

@cshearer1977, can you try to drop csp from renewable_carriers in your config file? This should work as a temporary fix.

@GbotemiB: Thanks for the suggestion. I deleted "csp" from the renewable carriers in the config file but got this error message:

KeyError: "None of [Index(['BR0 0 csp', 'BR0 1 csp', 'BR0 2 csp', 'BR0 3 csp', 'BR0 4 csp',\n 'BR0 5 csp', 'BR0 6 csp', 'BR0 7 csp', 'BR0 8 csp', 'BR0 9 csp',\n 'BR1 0 csp', 'BR2 0 csp', 'BR3 0 csp', 'BR4 0 csp', 'BR5 0 csp',\n 'BR6 0 csp'],\n dtype='object', name='Bus')] are in the [index]"

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 nc files created in the previous run.

GbotemiB avatar Sep 03 '24 22:09 GbotemiB

@cshearer1977, can you try to drop csp from renewable_carriers in your config file? This should work as a temporary fix.

@GbotemiB: Thanks for the suggestion. I deleted "csp" from the renewable carriers in the config file but got this error message: KeyError: "None of [Index(['BR0 0 csp', 'BR0 1 csp', 'BR0 2 csp', 'BR0 3 csp', 'BR0 4 csp',\n 'BR0 5 csp', 'BR0 6 csp', 'BR0 7 csp', 'BR0 8 csp', 'BR0 9 csp',\n 'BR1 0 csp', 'BR2 0 csp', 'BR3 0 csp', 'BR4 0 csp', 'BR5 0 csp',\n 'BR6 0 csp'],\n dtype='object', name='Bus')] are in the [index]" 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 nc files created in the previous run.

Thanks. Before rerunning the model I did snakemake -j 1 clean - that wouldn't do it? I need to delete the resources folder completely?

cshearer1977 avatar Sep 03 '24 22:09 cshearer1977

It should delete the content, but if it doesn't, you can just delete the resources folder.

GbotemiB avatar Sep 03 '24 23:09 GbotemiB

Many thanks everyone! I understand with Katia this pr needs to be merged (see the other pr). I'll revise and fix it soon.

@cshearer1977 does this pr fix most of your issues? I understand you still have some issues with csp anyway? Could you please summarize your experience?

davide-f avatar Sep 05 '24 07:09 davide-f

Many thanks everyone! I understand with Katia this pr needs to be merged (see the other pr). I'll revise and fix it soon.

@cshearer1977 does this pr fix most of your issues? I understand you still have some issues with csp anyway? Could you please summarize your experience?

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: KeyError: "['BR6 0 csp'] not in index"

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: @KeyError: "None of [Index(['BR0 0 csp', 'BR0 1 csp', 'BR0 2 csp', 'BR0 3 csp', 'BR0 4 csp',\n 'BR0 5 csp', 'BR0 6 csp', 'BR0 7 csp', 'BR0 8 csp', 'BR0 9 csp',\n 'BR1 0 csp', 'BR2 0 csp', 'BR3 0 csp', 'BR4 0 csp', 'BR5 0 csp',\n 'BR6 0 csp'],\n dtype='object', name='Bus')] are in the [index]"

Given your recent commit I tried to clone the repo again today and retry (without deleting csp) and got the same error as before: KeyError: "['BR6 0 csp'] not in index"

Here is more of the error message:

File "/Users/christines/opt/miniconda3/envs/pypsa-earth/lib/python3.10/site-packages/pandas/core/indexes/base.py", line 6252, in _raise_if_missing
    raise KeyError(f"{not_found} not in index")
KeyError: "['BR6 0 csp'] not in index"
[Thu Sep  5 11:42:59 2024]
Error in rule add_extra_components:
    jobid: 3
    input: networks/elec_s_10.nc, data/costs.csv
    output: networks/elec_s_10_ec.nc
    log: logs/add_extra_components/elec_s_10.log (check log file(s) for error details)

RuleException:
CalledProcessError in file [/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/Snakefile](https://file+.vscode-resource.vscode-cdn.net/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/Snakefile), line 756:
Command 'set -euo pipefail;  [/Users/christines/opt/miniconda3/envs/pypsa-earth/bin/python3.10](https://file+.vscode-resource.vscode-cdn.net/Users/christines/opt/miniconda3/envs/pypsa-earth/bin/python3.10) [/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/.snakemake/scripts/tmpmm8o1xf0.add_extra_components.py](https://file+.vscode-resource.vscode-cdn.net/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/.snakemake/scripts/tmpmm8o1xf0.add_extra_components.py)' returned non-zero exit status 1.
  File "/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/Snakefile", line 756, in __rule_add_extra_components
  File "/Users/christines/opt/miniconda3/envs/pypsa-earth/lib/python3.10/concurrent/futures/thread.py", line 58, in run
Shutting down, this might take some time.
Exiting because a job execution failed. Look above for 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!

cshearer1977 avatar Sep 05 '24 21:09 cshearer1977

Many thanks everyone! I understand with Katia this pr needs to be merged (see the other pr). I'll revise and fix it soon. @cshearer1977 does this pr fix most of your issues? I understand you still have some issues with csp anyway? Could you please summarize your experience?

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: KeyError: "['BR6 0 csp'] not in index"

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: @KeyError: "None of [Index(['BR0 0 csp', 'BR0 1 csp', 'BR0 2 csp', 'BR0 3 csp', 'BR0 4 csp',\n 'BR0 5 csp', 'BR0 6 csp', 'BR0 7 csp', 'BR0 8 csp', 'BR0 9 csp',\n 'BR1 0 csp', 'BR2 0 csp', 'BR3 0 csp', 'BR4 0 csp', 'BR5 0 csp',\n 'BR6 0 csp'],\n dtype='object', name='Bus')] are in the [index]"

Given your recent commit I tried to clone the repo again today and retry (without deleting csp) and got the same error as before: KeyError: "['BR6 0 csp'] not in index"

Here is more of the error message:

File "/Users/christines/opt/miniconda3/envs/pypsa-earth/lib/python3.10/site-packages/pandas/core/indexes/base.py", line 6252, in _raise_if_missing
    raise KeyError(f"{not_found} not in index")
KeyError: "['BR6 0 csp'] not in index"
[Thu Sep  5 11:42:59 2024]
Error in rule add_extra_components:
    jobid: 3
    input: networks/elec_s_10.nc, data/costs.csv
    output: networks/elec_s_10_ec.nc
    log: logs/add_extra_components/elec_s_10.log (check log file(s) for error details)

RuleException:
CalledProcessError in file [/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/Snakefile](https://file+.vscode-resource.vscode-cdn.net/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/Snakefile), line 756:
Command 'set -euo pipefail;  [/Users/christines/opt/miniconda3/envs/pypsa-earth/bin/python3.10](https://file+.vscode-resource.vscode-cdn.net/Users/christines/opt/miniconda3/envs/pypsa-earth/bin/python3.10) [/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/.snakemake/scripts/tmpmm8o1xf0.add_extra_components.py](https://file+.vscode-resource.vscode-cdn.net/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/.snakemake/scripts/tmpmm8o1xf0.add_extra_components.py)' returned non-zero exit status 1.
  File "/Users/christines/Desktop/workspace/PyPSA/pypsa-earth/Snakefile", line 756, in __rule_add_extra_components
  File "/Users/christines/opt/miniconda3/envs/pypsa-earth/lib/python3.10/concurrent/futures/thread.py", line 58, in run
Shutting down, this might take some time.
Exiting because a job execution failed. Look above for 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. You can debug the script. If you adapt this line: https://github.com/pypsa-meets-earth/pypsa-earth/blob/1bd7f84f1f9b0e081d790ffa198729076fd923b6/scripts/add_extra_components.py#L271 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 avatar Sep 06 '24 10:09 davide-f

Many thanks everyone! I understand with Katia this pr needs to be merged (see the other pr). I'll revise and fix it soon.

@cshearer1977 does this pr fix most of your issues? I understand you still have some issues with csp anyway? Could you please summarize your experience?

@davide-f @GbotemiB is this PR ready to be merged? Happy to support if needed 🙂

ekatef avatar Sep 12 '24 13:09 ekatef

Many thanks everyone! I understand with Katia this pr needs to be merged (see the other pr). I'll revise and fix it soon. @cshearer1977 does this pr fix most of your issues? I understand you still have some issues with csp anyway? Could you please summarize your experience?

@davide-f @GbotemiB is this PR ready to be merged? Happy to support if needed 🙂

From my end, it is ready to be merged.

GbotemiB avatar Sep 12 '24 15:09 GbotemiB

Merging to keep the model fully functional before the merge Thanks a lot @davide-f for taking care about that! 😄

ekatef avatar Sep 16 '24 12:09 ekatef