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 0.7 uses new object type for CRS #19

Closed
blakerosenthal opened this issue Feb 20, 2020 · 8 comments
Closed

Geopandas 0.7 uses new object type for CRS #19

blakerosenthal opened this issue Feb 20, 2020 · 8 comments

Comments

@blakerosenthal
Copy link

Description of the bug

Geopandas 0.7 changed the CRS type from a str to a pyproj.CRS class instance in this commit. This causes the check in

if (gdf.crs is not None) and ('proj' in gdf.crs) \
to fail with the below error.

Environment

  • Python version: 3.8

  • OSMnet version: 0.1.5

Paste the code that reproduces the issue here:

osm.pdna_network_from_bbox(lat_min=ymin, lng_min=xmin, lat_max=ymax, lng_max=xmax)

Paste the error message (if applicable):

  File "/usr/local/anaconda3/envs/nbtest6/lib/python3.8/site-packages/netbuffer/core/network.py", line 90, in get_osm_network
    network = osm.pdna_network_from_bbox(lat_min=ymin,
  File "/usr/local/anaconda3/envs/nbtest6/lib/python3.8/site-packages/pandana/loaders/osm.py", line 49, in pdna_network_from_bbox
    nodes, edges = network_from_bbox(lat_min=lat_min, lng_min=lng_min,
  File "/usr/local/anaconda3/envs/nbtest6/lib/python3.8/site-packages/osmnet/load.py", line 850, in network_from_bbox
    nodes, ways, waynodes = ways_in_bbox(
  File "/usr/local/anaconda3/envs/nbtest6/lib/python3.8/site-packages/osmnet/load.py", line 654, in ways_in_bbox
    osm_net_download(lat_max=lat_max, lat_min=lat_min, lng_min=lng_min,
  File "/usr/local/anaconda3/envs/nbtest6/lib/python3.8/site-packages/osmnet/load.py", line 136, in osm_net_download
    geometry_proj, crs_proj = project_geometry(polygon,
  File "/usr/local/anaconda3/envs/nbtest6/lib/python3.8/site-packages/osmnet/load.py", line 448, in project_geometry
    gdf_proj = project_gdf(gdf, to_latlong=to_latlong)
  File "/usr/local/anaconda3/envs/nbtest6/lib/python3.8/site-packages/osmnet/load.py", line 485, in project_gdf
    if (gdf.crs is not None) and ('proj' in gdf.crs) \
TypeError: argument of type 'CRS' is not iterable
@blakerosenthal blakerosenthal changed the title Geopandas 0.7 requires new object type for CRS Geopandas 0.7 uses new object type for CRS Feb 21, 2020
@smmaurer
Copy link
Member

Interesting, thanks for reporting this @blakerosenthal!

I'm not very familiar with the codebase, but this is happening inside a function called osmnet.load.project_gdf() -- docstring here.

Looks like the function is used internally, to project a lat-lon bounding box into the UTM zone of its centroid in order to calculate the area and potentially subdivide it into smaller units for the data download query in osmnet.load.osm_net_download(). The code we need to fix catches input that's already in a UTM projection, and returns it without doing anything.

I'm thinking we have a couple of options here:

  1. Update the code so it can detect UTM projections using either the old or new gdf.crs format.

  2. Get rid of the check entirely -- i.e., remove lines load.py#L487-L490.

Option 2 seems appealing. (a) It's simpler. (b) The computational cost is minimal: we might end up calculating the centroid of a unary union without needing to. (c) It's actually closer to the behavior described in the function's docstring -- which says the input will be projected to the UTM zone of its centroid, without mentioning that gdf's already in a UTM projection will be returned untouched.

Thoughts, @sablanchard?

And some additional info about the Geopandas change, courtesy of @PyMap: https://jorisvandenbossche.github.io/blog/2020/02/11/geopandas-pyproj-crs/

@knaaptime
Copy link
Contributor

the version of this function in osmnx has been updated and could be used as a reference for updating this one :)

@knaaptime
Copy link
Contributor

in the interest of a speedy fix, I could do a PR that copies over the updated function from osmnx, if folks are ok with that?

with that said, though, this function is super handy and applicable to a lot of workflows (we also use it in both segregation and tobler, for example). Would there be benefit into putting it somewhere slightly more centralized? I'm not sure if geopandas or pyproj would be interested, but libpysal could also be an option.

Originally, we were importing it from osmnx, but that added a relatively heavy dependency for only a single function (i.e. it caused some networkx headaches elsewhere). But if folks want to use this in lots of places, it might benefit from a single shared implementation

@gboeing @jorisvandenbossche @ljwolf @snowman2

@snowman2
Copy link

snowman2 commented Mar 4, 2020

I think that a property on the pyproj CRS class that essentially does is_utm would be useful. I will add it to the list for the 2.6 release.

@snowman2
Copy link

snowman2 commented Mar 4, 2020

pyproj4/pyproj#562

@smmaurer
Copy link
Member

smmaurer commented Mar 4, 2020

This all sounds great!

Yes, @knaaptime, if you'd be interested in implementing a quick fix to OSMNet, I'll get it merged and released. I just set up a clean dev branch, which you can use as the target for the PR.

I was talking about this offline with @sablanchard, and his inclination is to retain the same behavior as OSMNx and return the GeoDataFrame untouched if it's already in a UTM projection. Let's mention this in the docstring, though. And I gather from OSMNx that one of the rationales for this is that the zone calculation isn't appropriate for polar latitudes.

Thanks all!

@smmaurer
Copy link
Member

Fixed in PR #21 and released in PR #22/#23 v0.1.6.

@snowman2
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants