Skip to content
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

Add endogenous export #1201

Merged
merged 12 commits into from
Nov 28, 2024
Merged

Conversation

energyLS
Copy link
Collaborator

@energyLS energyLS commented Nov 18, 2024

Closes # (if applicable).

Changes proposed in this Pull Request

This PR adds the possibility of endogenous exports to the model. Previously, it was only possible to set a fixed volume of hydrogen exports (e.g. 10 TWh). This feature enables the possibility, to export hydrogen which is covered by a certain market price for hydrogen set in the config.

In detail, it adds these options:

  • endogenous: A boolean flag indicating whether the export demand is endogenously determined by the model. If true, the export demand is determined by the model.
  • endogenous_price: The market price (in EUR/MWh) at which hydrogen for endogenous exports is sold. This parameter is only considered if endogenous is set to true.

Checklist

  • 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.
  • 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.

@energyLS
Copy link
Collaborator Author

@ekatef I have added this PR and the config options are added in config.default.yaml. Do I also need to add the options in the config.tutorial.yaml or test/ for specific tests, or is that not required anymore? What do we test and what not?

@hazemakhalek I think you have previously come up with a similar feature in pypsa-meets-earth/pypsa-earth-sec@main...bright. Can you point me to it, so I can outline the differences? Also, what do you think of this proposal?

Copy link
Member

@ekatef ekatef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great @energyLS! Sounds like a very nice enhancement 😄
Have added a couple of comments to the code, which are quite minor though.

As for the configs, config.tutorial currently contains a power only model, and no updates are needed here. Regarding test configurations, it would be great to add a new option to test/config.test_myopic.yaml. The reason is that testing configs are build upon the tutorial config, so all the cross-sectoral parameters must be specified in the test configuration directly.

Could you also please update a version of the config in the tutorial and the default configs?

config.default.yaml Outdated Show resolved Hide resolved
scripts/add_export.py Outdated Show resolved Hide resolved
scripts/add_export.py Outdated Show resolved Hide resolved
@energyLS
Copy link
Collaborator Author

Dear @ekatef, thanks for reviewing! I have:

  • Revised your suggestions to streamline the if-else condition: 0d67965
  • Grouped endogenous and exogenous parameters together: cd0fd4d. Or do you mean it could be grouped in a sense, that all exogenous/endogenous params are a subparameter? E.g. ["endogenous"]["endogenous_price"] and ["exogenous"]["h2export"] and so on?
  • Updated the versions of config.default.yaml and test/config.test_myopic.yaml: c6c7ebe
  • Added the options to the test/config.test_myopic.yaml: f76b16f. Should the new option be set to true, to actually test it? Or do we keep it as it is (endogenous=False) and "only" test on our standard configuration?

And since we haven't documented the config options of the sector-coupled version in https://pypsa-earth.readthedocs.io/en/latest/configuration.html yet, I guess there is nothing to add here at this point? Of course it should be done in general at one point :)

@ekatef
Copy link
Member

ekatef commented Nov 26, 2024

https://pypsa-earth.readthedocs.io/en/latest/configuration.html

Dear @ekatef, thanks for reviewing! I have:

  • Revised your suggestions to streamline the if-else condition: 0d67965
  • Grouped endogenous and exogenous parameters together: cd0fd4d. Or do you mean it could be grouped in a sense, that all exogenous/endogenous params are a subparameter? E.g. ["endogenous"]["endogenous_price"] and ["exogenous"]["h2export"] and so on?
  • Updated the versions of config.default.yaml and test/config.test_myopic.yaml: c6c7ebe
  • Added the options to the test/config.test_myopic.yaml: f76b16f. Should the new option be set to true, to actually test it? Or do we keep it as it is (endogenous=False) and "only" test on our standard configuration?

And since we haven't documented the config options of the sector-coupled version in https://pypsa-earth.readthedocs.io/en/latest/configuration.html yet, I guess there is nothing to add here at this point? Of course it should be done in general at one point :)

Hey @energyLS, thanks a lot for implementing the changes!

As a technical comment, not need to specify the particular commits. We trust you! 😉

Agree that it feels worth to have an advanced version tested for each functionality, while I'm also confident in your expertise for sector-couple modeling. To me, the test configuration looks good as it is now.

Regarding documenting the config options for the sector-coupled part you are indeed absolutely right. That is clearly out of scope for this PR, but it would be definitely useful to open an issue on that. Would you mind doing that? 🙏🏽

The only very minor point left is to update the config version also in the tutorial config. Also, please feel free to add a release note! That is quite a significant update, and it would be great to have it mentioned in the release.

After that I think the PR is ready to merge, unless @davide-f or @hazemakhalek have any comments.

@hazemakhalek
Copy link
Collaborator

I trust you both. Go ahead if you feel it's ready

@energyLS
Copy link
Collaborator Author

https://pypsa-earth.readthedocs.io/en/latest/configuration.html

Dear @ekatef, thanks for reviewing! I have:

  • Revised your suggestions to streamline the if-else condition: 0d67965
  • Grouped endogenous and exogenous parameters together: cd0fd4d. Or do you mean it could be grouped in a sense, that all exogenous/endogenous params are a subparameter? E.g. ["endogenous"]["endogenous_price"] and ["exogenous"]["h2export"] and so on?
  • Updated the versions of config.default.yaml and test/config.test_myopic.yaml: c6c7ebe
  • Added the options to the test/config.test_myopic.yaml: f76b16f. Should the new option be set to true, to actually test it? Or do we keep it as it is (endogenous=False) and "only" test on our standard configuration?

And since we haven't documented the config options of the sector-coupled version in https://pypsa-earth.readthedocs.io/en/latest/configuration.html yet, I guess there is nothing to add here at this point? Of course it should be done in general at one point :)

Hey @energyLS, thanks a lot for implementing the changes!

As a technical comment, not need to specify the particular commits. We trust you! 😉

Agree that it feels worth to have an advanced version tested for each functionality, while I'm also confident in your expertise for sector-couple modeling. To me, the test configuration looks good as it is now.

Regarding documenting the config options for the sector-coupled part you are indeed absolutely right. That is clearly out of scope for this PR, but it would be definitely useful to open an issue on that. Would you mind doing that? 🙏🏽

The only very minor point left is to update the config version also in the tutorial config. Also, please feel free to add a release note! That is quite a significant update, and it would be great to have it mentioned in the release.

After that I think the PR is ready to merge, unless @davide-f or @hazemakhalek have any comments.

Dear @ekatef,
thanks for the feedback! I have added the new feature to the release notes, and adjusted the config version of the tutorial config. Also, I have set up a new issue for the documentation: #1208

I think everthing is done now and ready to merge, however the CI currently fails due to a problem not related with this PR:
https://github.com/pypsa-meets-earth/pypsa-earth/actions/runs/12046200910/job/33586189025?pr=1201#step:15:5.
Once that is fixed, we're good to go!

@ekatef
Copy link
Member

ekatef commented Nov 27, 2024

Dear @ekatef, thanks for the feedback! I have added the new feature to the release notes, and adjusted the config version of the tutorial config. Also, I have set up a new issue for the documentation: #1208

I think everthing is done now and ready to merge, however the CI currently fails due to a problem not related with this PR: https://github.com/pypsa-meets-earth/pypsa-earth/actions/runs/12046200910/job/33586189025?pr=1201#step:15:5. Once that is fixed, we're good to go!

Hey @energyLS, perfect! Thanks a lot for the changes, and opening the issue on the documentation is highly appreciated 😄

Agree on the plan: let's fix the CI and merge.

@ekatef
Copy link
Member

ekatef commented Nov 28, 2024

Hey @energyLS CI has been fixed thanks to the recent work by @davide-f!! 🎉
Feel free to update your branch and re-run the workflows 😄

@ekatef ekatef merged commit ccc3c64 into pypsa-meets-earth:main Nov 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants