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

Remove version restriction on geopandas, and remove fiona optional dependency #488

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

Conversation

angusmcb
Copy link
Contributor

Summary

These changes fix the error which were keeping geopandas < 1.0
Notably, this also means fiona is no longer used, so removed from the optional dependency list
The only functional change is that it's now necessary to point to a specific shapefile when writing, rather than a directory to write to.
This contributes to #422
It completely duplicates #487

Tests and documentation

Mention of shapefiles writing updated.
Doctests updated.

Acknowledgement

By contributing to this software project, I acknowledge that I have reviewed the software quality assurance guidelines and that my contributions are submitted under the Revised BSD License.

@coveralls
Copy link

coveralls commented Mar 12, 2025

Coverage Status

coverage: 81.803% (+0.02%) from 81.788%
when pulling 3984ff0 on angusmcb:intersection-fix
into 102e2ad on USEPA:main.

Copy link
Collaborator

@kbonney kbonney left a comment

Choose a reason for hiding this comment

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

Hi @angusmcb, thank you for the well-written PR. I have a couple questions before merging:

  1. I noticed that in gis.rst that trailing 0s in the doctest outputs are removed. I am assuming that this is a feature of geopandas>=1.0 - can you confirm that?
  2. Can you explain the need to switch from shapefile folders to the individual ".shp" files? Is this a requirement of geopandas>=1?
  3. Why the change in geospatial.py? Re-reading through the function, we don't make any mutations to the B dataframe, so I don't think a copy is necessary.

@angusmcb
Copy link
Contributor Author

Thanks for the review ! To respond to your questions:

  1. Not absolutely sure why it's changed, but I think it's probably due to this: REF: trim trailing zeros in WKT to generate __repr__ geopandas/geopandas#3087 .
  2. I'm not sure why saving shapefiles into folders no longer works but it was giving errors that I couldn't see how to resolve otherwise. Looking in the geopandas documentation they have not updated it to say that it doesn't work. Under the hood geopandas isn't using fionas anymore - it uses pyogrio. Maybe this explains it?
  3. This is due to the following change: BUG/API: also preserve right index name in spatial join geopandas/geopandas#2144 When doing the sjoin one of the keys created is 'index_right' is no longer always called index_right, and if for example the index is called 'name' it will now be called 'name_right' instead. Therefore, it's necessary to remove any name from the index before the sjoin so that there is consistently an 'index_right'. The copy is made just before this so as not to change the index name in the original dataframe.

@angusmcb
Copy link
Contributor Author

And just to add on point 3 that it still worked with folders for shapefiles when writing the first time, but didn't overwrite existing shapefiles. This may be why it hasn't been removed from the geopandas documentation.

@kbonney
Copy link
Collaborator

kbonney commented Mar 27, 2025

Thanks for the followup @angusmcb.

  1. Gotcha!
  2. I wasn't able to reproduce this on my end. Saving/loading shapefiles from a folder works just fine. Could you create a minimal example of the problem and share the environment (eg pip list output).
  3. I understand - I pushed a change that just saves the original index, changes it to None, and then restores it at the end of the function. This prevents the duplication of B which could potentially be very large.

@kbonney
Copy link
Collaborator

kbonney commented Mar 27, 2025

And just to add on point 3 that it still worked with folders for shapefiles when writing the first time, but didn't overwrite existing shapefiles. This may be why it hasn't been removed from the geopandas documentation.

Ah, I understand what you mean by this now. Yes I run into the same error after running to_file twice. I think we want to stick with the folder approach, though, so we just need to change the syntax to include the folder and filename. EG: instead of gdf.to_file("my_gdf") do gdf.to_file("my_gdf/my_gdf.shp").

@angusmcb
Copy link
Contributor Author

Ok I've put in some folder-saving logic so the functionality should be back to how it used to be.

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