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

Added GeoPandas data interface #88

Merged
merged 12 commits into from Nov 3, 2017
Merged

Added GeoPandas data interface #88

merged 12 commits into from Nov 3, 2017

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Sep 14, 2017

Adds a GeoPandas interface allowing Path and Polygon types to wrap around geopandas dataframes directly. Here is a basic example:

https://anaconda.org/philippjfr/geopandas/notebook

Requires holoviews master and will likely be released at the same time as HoloViews 1.9.

@jbednar
Copy link
Member

@jbednar jbednar commented Sep 14, 2017

Excellent! Would it be difficult to make it work with Matt Rocklin's new cythonized geopandas equivalent as well, which appears to have vastly better performance than bare geopandas?

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Sep 14, 2017

Would it be difficult to make it work with Matt Rocklin's new cythonized geopandas equivalent as well, which appears to have vastly better performance than bare geopandas?

I assume that will eventually be folded into geopandas proper? As long as the API is similar/the same it should be trivial either way. That said if you really need the optimization then your data is likely larger than bokeh can comfortably render.

@mrocklin
Copy link

@mrocklin mrocklin commented Sep 14, 2017

No particular thoughts on this yet. We've been talking about getting coordinates out of GeoPandas a bit to speed up operations like this. I would not be surprised if we have a different API for this in the future, but I wouldn't wait on geopandas development.

I would be curious to know if you run into performance issues on larger datasets.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

Probably won't have time to add tests here, I'll do that at the same time as moving the iris interface from HoloViews. Otherwise this is ready to review @jbednar. The PR also removes a lot of plotting code relying instead on an approach that simply projects the element and then uses the HoloViews version of the plot to actually plot the element. This means that in most cases plotting classes consist solely of a declaration of the projection operation. There are now projection operations for most elements and I'll finish off better Image and RGB handling in #87.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

Can close #53 once merged.

@jbednar
Copy link
Member

@jbednar jbednar commented Nov 3, 2017

Looks good, apart from the failing test. It would be nice to add a sentence or two saying why to use GeoPandas, i.e. what you gain and tradeoffs there are if any.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

Very confused by the latest error, dask seems to be raising an error about np.cbrt not being defined:

  File "/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages/dask/array/ufunc.py", line 138, in <module>
    cbrt = ufunc(np.cbrt)
AttributeError: 'module' object has no attribute 'cbrt'

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

Looks like something in our travis setup is downgrading numpy to numpy 1.9.3-py27h7e35acb_3

@jbednar
Copy link
Member

@jbednar jbednar commented Nov 3, 2017

That's odd; surely all Numpy versions have cbrt. Something's fishy.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

No, np.cbrt is a new feature in NumPy 1.13 so it makes sense.

@jbednar
Copy link
Member

@jbednar jbednar commented Nov 3, 2017

Looks like cbrt was added in Numpy 1.10.0.

@jbednar
Copy link
Member

@jbednar jbednar commented Nov 3, 2017

So dask should be specifying numpy>=1.10, I guess?

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

Ah, sorry, I just searched the 1.13 changelog and didn't realize it listed all the changelogs.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

So dask should be specifying numpy>=1.10, I guess?

I suppose so, I think we may have a clash with some other dependency though, we'll see in a minute.

catawbasam
Copy link

@catawbasam catawbasam commented on 809490b Dec 7, 2017

Choose a reason for hiding this comment

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

ping?

philippjfr
Copy link
Member Author

@philippjfr philippjfr commented on 809490b Dec 7, 2017

Choose a reason for hiding this comment

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

It probably should, can't quite recall why this change was needed. I'll try it in a PR.

jbednar
Copy link
Member

@jbednar jbednar commented on 809490b Nov 3, 2017

Choose a reason for hiding this comment

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

That shouldn't be >=?

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

In the .travis.yml you mean? I prefer pinning specific versions there, but it probably doesn't make much of a difference.

@jbednar
Copy link
Member

@jbednar jbednar commented Nov 3, 2017

It's a tradeoff, I guess, between mistakenly thinking that a change we made caused a problem, when really it was a new release of numpy that did it, versus not realizing that users will have problems from new numpy releases when they do happen.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Nov 3, 2017

Will go ahead and merge this so I can move on to #87.

@philippjfr philippjfr merged commit 27db4b7 into master Nov 3, 2017
2 of 3 checks passed
@philippjfr philippjfr deleted the geopandas_interface branch Jan 13, 2018
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

4 participants