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

Landlock country fix alternative #383

Merged
merged 3 commits into from Jun 28, 2022
Merged

Landlock country fix alternative #383

merged 3 commits into from Jun 28, 2022

Conversation

FabianHofmann
Copy link
Contributor

@FabianHofmann FabianHofmann commented Jun 23, 2022

Closes #382
closes #265

Changes proposed in this Pull Request

Checklist

  • 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 envs/environment.docs.yaml.
  • Changes in configuration options are added in all of config.default.yaml, config.tutorial.yaml, and test/config.test1.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.

@FabianHofmann FabianHofmann force-pushed the landlock-fix-alterative branch 2 times, most recently from c4e7fbf to 1db3d62 Compare June 23, 2022 13:01
@pz-max
Copy link
Collaborator

pz-max commented Jun 23, 2022

Looks much more compact. Great! :)
Hope the CI runs through.
Testing it on these cases might help checking if its stable:

  • ["AT"] # landlock
  • ["AT", "CZ"] # multiple countries

@FabianHofmann
Copy link
Contributor Author

My internet in the train is terrible. @pz-max could you do me the favour and try your config settings with the up to date version?

@pz-max
Copy link
Collaborator

pz-max commented Jun 23, 2022

My internet in the train is terrible. @pz-max could you do me the favour and try your config settings with the up to date version?

@FabianHofmann I am now in the bus for 18h with okish internet connection. I tried to run the branch for both cases ['AT'] and ['AT','CZ']. The following error appeared:

File "/home/max/OneDrive/PHD-Flexibility/07_pypsa-africa/0github/pypsa-africa/pypsa-eur-test4/pypsa-eur/.snakemake/scripts/tmp1hvdqnee.base_network.py", line 581, in base_network
_set_countries_and_substations(n, config, country_shapes, offshore_shapes)
File "/home/max/OneDrive/PHD-Flexibility/07_pypsa-africa/0github/pypsa-africa/pypsa-eur-test4/pypsa-eur/.snakemake/scripts/tmp1hvdqnee.base_network.py", line 398, in _set_countries_and_substations
offshore_shapes = gpd.read_file(offshore_shapes).set_index('name')['geometry']
File "/home/max/anaconda3/envs/pypsa-eur-landlock/lib/python3.9/site-packages/pandas/util/_decorators.py", line 311, in wrapper
return func(*args, **kwargs)
File "/home/max/anaconda3/envs/pypsa-eur-landlock/lib/python3.9/site-packages/pandas/core/frame.py", line 5494, in set_index
raise KeyError(f"None of {missing} are in the columns")
KeyError: "None of ['name'] are in the columns"

I see one problem with the here suggested solution. I think we cannot just provide an empty dataframe that solves everything because we use the offshore_shapes and later the offshore_regions to execute functions. If data such as columns or the values in the dataframes are missing errors might occur as above.

I think to avoid issues with the import:

  • We need as in the previous drafted PR file size check & and write out of empty files for the offshore stuff (not to break the workflow)

To avoid any operational errors, e.g. when geopandas function are intended to be executed on the empty file:

  • We might need to add the case distinctions as previously suggested.

In future, it might make also sense to distentangle any onshore & offshore functions e.g. within the script or even with the rules (build_shapes -> build_offshore_shape + build_onshore_shape).... Atm some of them are mixxed up. But this is probably only a nice to have.

@FabianHofmann FabianHofmann force-pushed the landlock-fix-alterative branch 2 times, most recently from 11257d7 to 7bc625d Compare June 24, 2022 12:30
@FabianHofmann
Copy link
Contributor Author

@pz-max it appears that the new geopandas version supports writing out empty geojsons! The only downside: the reimported empty dataframe has lost all columns but the geometry column. Forcing a reindex after the imports of offshore shapes makes everything happen.

It runs through for me now. However, one has to disable offshore tech if no offshore shapes are existent. Otherwise the workflow will raise an error.

I prefer this implementation over the other approach as it is cleaner. Especially, getting rid of the save_to_geojson function is a point I would like to have. Let me know if you have second thoughts.

Copy link
Member

@fneum fneum left a comment

Choose a reason for hiding this comment

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

This solution looks good! Approved to be merged.

scripts/_helpers.py Show resolved Hide resolved
@fneum fneum closed this Jun 28, 2022
@fneum fneum reopened this Jun 28, 2022
@fneum fneum enabled auto-merge June 28, 2022 05:49
@fneum fneum merged commit 452d8cd into master Jun 28, 2022
@fneum fneum deleted the landlock-fix-alterative branch June 28, 2022 06:02
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.

Support selection of only landlocked countries
3 participants