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

Inlining hilbertcurve dependency or getting it on conda #51

Open
philippjfr opened this issue Aug 15, 2020 · 5 comments
Open

Inlining hilbertcurve dependency or getting it on conda #51

philippjfr opened this issue Aug 15, 2020 · 5 comments
Labels
enhancement New feature or request

Comments

@philippjfr
Copy link
Member

philippjfr commented Aug 15, 2020

The spatial indexing in spatialpandas is using the hilbertcurve package. The implementation is a tiny package of about ~150 lines of code, which is currently MIT licensed. Since this is a well established implementation of a pretty standard algorithm I don't think we will gain a whole lot from depending directly on this package since there are unlikely to be bug fixes in the actual code. At the same time as long as we get the dependency on conda once we also don't need to worry much about having to wait for a release. On balance my vote is to simply inline the code and potentially optimize it with numba later if that's worthwhile. In the short term this will simplify my short term pain in setting up the build infrastructure. Overall I also think the conda ecosystem suffers from tiny packages so I'm not entirely enthusiastic about adding it conda-forge and potentially defaults (eventually).

Cc: @jonmmease @jbednar

@jbednar
Copy link
Member

jbednar commented Aug 15, 2020

Inlining sounds like a good approach here.

@jonmmease
Copy link
Collaborator

For context, when I started this I used the hilbertcurve package (https://github.com/galtay/hilbertcurve) as a reference, but not a runtime dependency. I developed a numba implementation of the hilbert curve calculations in https://github.com/holoviz/spatialpandas/blob/master/spatialpandas/spatialindex/hilbert_curve.py, and this is what is used by the RTree implementation in https://github.com/holoviz/spatialpandas/blob/master/spatialpandas/spatialindex/rtree.py.

I did use the hilbertcurve package as a testing dependency to make sure my implementation produced identical results (See https://github.com/holoviz/spatialpandas/blob/master/tests/spatialindex/test_hilbert_curve.py).

I don't have a strong opinion on inlining vs depending, but wanted to point out that it's only used in testing.

@jbednar
Copy link
Member

jbednar commented Aug 15, 2020

Sounds like a good reason to vendor it in; we don't want any changes to that library to break our tests; it's effectively just some reference code.

@philippjfr
Copy link
Member Author

Thanks @jonmmease, totally missed that.

@philippjfr
Copy link
Member Author

For now I'm happy to install it separately on CI.

@jlstevens jlstevens added the enhancement New feature or request label Dec 7, 2020
@jlstevens jlstevens modified the milestones: v0.3.6, v0.3.7 Dec 7, 2020
@ianthomas23 ianthomas23 removed this from the v0.3.7 milestone Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants