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

Fix adding H2 export ports for land-locked countries #1229

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ekatef
Copy link
Member

@ekatef ekatef commented Dec 6, 2024

Changes proposed in this Pull Request

There may be countries without see ports at all, or with ports which are not suitable for export H2. In this case adding ports for export H2 should be treated differently.

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.

@yerbol-akhmetov
Copy link
Collaborator

Thanks, @ekatef. Can you please look into issue #1231? It is related to H2 export. Basically, duplicate entries in hydrogen_buses_ports in add_export rule preven from adding link that satisfies H2 export loads. I have resolved the issue locally by removing the duplicates in line 70 of add_export.py:

    # drop duplicates
    hydrogen_buses_ports = hydrogen_buses_ports.drop_duplicates()

    return hydrogen_buses_ports

Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.
An alternative approach would be to add an option in export to specify whether this feature shall be included or not.
The risk of this handling is that a user may specify an export capacity but as the dataframe is empty, that is not met, thus leading to misunderstanding.

An option to disable the feature would be interesting.
This option has however architecture changes and a comment by @Eddy-JV , @hazemakhalek or @energyLS is advisable

@ekatef
Copy link
Member Author

ekatef commented Dec 9, 2024

Thanks, @ekatef. Can you please look into issue #1231? It is related to H2 export. Basically, duplicate entries in hydrogen_buses_ports in add_export rule preven from adding link that satisfies H2 export loads. I have resolved the issue locally by removing the duplicates in line 70 of add_export.py:

    # drop duplicates
    hydrogen_buses_ports = hydrogen_buses_ports.drop_duplicates()

    return hydrogen_buses_ports

Thanks a lot for pointing out @yerbol-akhmetov. Agree that the issues are somehow related but it feels like another "extreme case". I have tested the model for Kazakhstan (I have a test setup for the country due to some project-related reasons), and our dataset does not contain any ports which could be used to export H2. So, in this case the troubles are caused by the fact that there is no export ports, while #1231 seems to relate to the case when there are too many ports.

@ekatef
Copy link
Member Author

ekatef commented Dec 9, 2024

This PR looks good to me. An alternative approach would be to add an option in export to specify whether this feature shall be included or not. The risk of this handling is that a user may specify an export capacity but as the dataframe is empty, that is not met, thus leading to misunderstanding.

An option to disable the feature would be interesting. This option has however architecture changes and a comment by @Eddy-JV , @hazemakhalek or @energyLS is advisable

Thank you @davide-f Absolutely agree that it could be probably tackled in a more consistent way, probably also keeping in mind #1231 issue mentioned by @yerbol-akhmetov

@Eddy-JV @hazemakhalek @energyLS it would be very much appreciated to have your review

@davide-f
Copy link
Member

Ah apologies, I was in review mode and agree there is potential duplication of PRs.
Feel free to coordinate yerbol and katia as you feel best

@davide-f
Copy link
Member

Given the merge of the other PR, this could be resolved.
It looks like there aren't conflicts, do you plan changes here?

@ekatef
Copy link
Member Author

ekatef commented Dec 12, 2024

Given the merge of the other PR, this could be resolved. It looks like there aren't conflicts, do you plan changes here?

No, personally I'm happy with the PR as it is. May be just worth to have a brief discussion with the sector-coupled team today

@ekatef
Copy link
Member Author

ekatef commented Dec 12, 2024

Update: as discussed with @Eddy-JV, the current implementation should also account the demand for export H2. If there is the demand set but no export ports to satisfy it, the model will be unfeasible. So, this point must be checked and addresses in this PR

Many thanks for the discussion @Eddy-JV!

@davide-f
Copy link
Member

What about instead of having a special fix for land-locked country we plan for:

  1. having an option to enable/disable the export feature
  2. when a land-locked country-only is selected [or rather no ports are found available], then throw an error suggesting to disable the export feature?

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