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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes incompatibility with GeoPandas 0.7 #21

Merged
merged 2 commits into from
Jul 9, 2020
Merged

Conversation

knaaptime
Copy link
Contributor

this resolves #19 (that's the only change... sorry about the formatting 馃槵)

@knaaptime
Copy link
Contributor Author

(closed the other PR cause you wanted this in the dev branch)

@smmaurer
Copy link
Member

smmaurer commented Jul 1, 2020

Many thanks @knaaptime, this looks really good!

I'd like @sablanchard's thoughts here too. Some discussion items:

  1. I know we're just following OSMNx, but changing from project_gdf(gdf, to_latlong, verbose) to project_gdf(gdf, to_crs, to_latlong) will break any downstream code that passes to_latlong positionally, or passes the verbose argument. All in all I'm on board -- it doesn't break anything internally, it's not a big change, and it's nice to follow OSMNx. But wanted to bring it up.

  2. Do we anticipate any problems requiring GeoPandas 0.7? From their changelog I see that it doesn't support Python 2.7. I think it would fine for OSMNet to drop support for Python 2.7 at this point, if @samblanchard is ok with it. The better GeoPandas functionality seems worth it. (We could also add code detecting the GeoPandas version and adjusting the logic, but that seems like more trouble than it's worth.)

  3. The Travis tests don't seem to be running -- i'll look into that.

  4. The code reformatting is not really a problem, but it would be nice to avoid if it's not too much trouble, just because it makes it harder to follow the code history. Maybe try downloading a clean copy of the file from GitHub, using a different text editor to paste in the changes, and committing that copy? But no worries if that's a pain.

Next steps:

After we resolve these, I can merge the PR and then open a new one for release prep -- bumping the OSMNet version number to 0.2.1 and taking care of any cleanup that's accumulated since the last release.

Then I'll merge that, tag a release in GitHub, and post it on PyPI and Conda Forge!

@smmaurer
Copy link
Member

smmaurer commented Jul 2, 2020

Followup: @sablanchard replied separately and is fine with points 1 (minor breaking of the API) and 2 (dropping Python 2.7 support). My understanding is that if we get the metadata right on PyPI and Conda Forge, the package managers will automatically install the appropriate older version of OSMNet for older versions of Python. So i'll do some tests to make sure that's working, when we get to it.

@smmaurer
Copy link
Member

smmaurer commented Jul 8, 2020

Well, the Travis tests are being initiated correctly, they're just not showing up in GitHub:
https://travis-ci.org/github/udst/osmnet/builds/703597318

Everything is failing, but the failures are not related to the substance of this PR, I don't think. My inclination is to go ahead and merge this, and fix Travis as part of the release prep work.

@smmaurer smmaurer merged commit dd2fee6 into UDST:dev Jul 9, 2020
@smmaurer smmaurer changed the title Project fix Fixes incompatibility with GeoPandas 0.7 Jul 9, 2020
@smmaurer smmaurer mentioned this pull request Jul 10, 2020
1 task
@smmaurer smmaurer self-requested a review July 10, 2020 19:38
@smmaurer smmaurer mentioned this pull request Jul 14, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants