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

GeoPandas: Migrate the engine from fiona to pyogrio #3231

Closed
seisman opened this issue May 7, 2024 · 5 comments · Fixed by #3247
Closed

GeoPandas: Migrate the engine from fiona to pyogrio #3231

seisman opened this issue May 7, 2024 · 5 comments · Fixed by #3247
Labels
enhancement Improving an existing feature
Milestone

Comments

@seisman
Copy link
Member

seisman commented May 7, 2024

GeoPandas v1.0.0-alpha1 has been released (https://geopandas.org/en/latest/docs/changelog.html#version-1-0-0-alpha1-apr-13-2024). The biggest change is that the default I/O engine is changed from Fiona to pyogrio.

So, when geopandas v1.0.0 is released, PyGMT will fail to work, because:

  1. Fiona is no longer a required dependency of geopandas, so it's not installed by default
  2. The pyogrio engine doesn't support scheme parameter so some codes will break.

I think we should start the migration now following the instructions.

  1. Add pyogrio as a optional dependency since geopandas 0.x doesn't install it by default
  2. Add engine="pyogrio" to use the pyogrio
  3. Cleanup the codes and avoid using fiona

Thoughts @GenericMappingTools/pygmt-maintainers especially @weiji14?

@seisman seisman added the discussions Need more discussion before taking further actions label May 7, 2024
@seisman
Copy link
Member Author

seisman commented May 7, 2024

We also have some mysterious failures with Fiona as mentioned in #3180 (comment). Migrating from Fiona to pyogrio also help us avoid such failures.

@weiji14
Copy link
Member

weiji14 commented May 7, 2024

Not sure if we can migrate completely away from fiona yet, at least in terms of writing out OGR_GMT files. Pyogrio doesn't list OGR_GMT under https://pyogrio.readthedocs.io/en/latest/supported_formats.html#full-read-and-write-support, though it seems to depend on what GDAL was compiled with. We might just need to test things out and see first.

@seisman
Copy link
Member Author

seisman commented May 7, 2024

OGR_GMT is built-in by default in GDAL (https://gdal.org/drivers/vector/gmt.html), so we should be safe unless some package distributors decide to disable OGR_GMT explicitly.

@weiji14
Copy link
Member

weiji14 commented May 9, 2024

Ok, opened a proof of concept PR at #3237 to switch from fiona to pyogrio. The way it works is that we get a GeoJSON representation from __geo_interface__, which pyogrio reads into a geopandas.GeoDataFrame, and that gets output to an OGR_GMT file.

Downside is that this means geopandas would become a stronger optional dependency. Before, someone could install just fiona without geopandas and plot shapely objects, but now, doing so via pyogrio would require geopandas.

@seisman
Copy link
Member Author

seisman commented May 9, 2024

Maybe we can try geopandas first, then pyogrio, then fiona? But we may still see fiona errors reported in #3180 (comment).

@seisman seisman added enhancement Improving an existing feature and removed discussions Need more discussion before taking further actions labels May 15, 2024
@seisman seisman added this to the 0.13.0 milestone May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
2 participants