Implement numpy support #24
Conversation
|
@TheBB nice work! I'm wondering if we could adjust the TravisCI config to test both cases though to make sure one doesn't break the other without noticing. |
|
@TheBB this is great! I just blasted through a conversion of about 80 million points in a matter of seconds. This saved me days if not weeks of processing time! |
|
@TheBB I'd really like to merge this, but we need to be testing both code paths, otherwise I can't accept this PR... |
|
Yes, I saw that. I'm not sure how best to implement such a thing. |
|
If there is still interest on this, I can work on the |
|
@Juanlu001 sounds good, thanks for looking into this! :) |
TheBB
force-pushed
the
numpy
branch
3 times, most recently
from
61d9c83 to
b2ce6fb
Compare
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
========================================
Coverage ? 93.5%
========================================
Files ? 3
Lines ? 154
Branches ? 0
========================================
Hits ? 144
Misses ? 10
Partials ? 0
Continue to review full report at Codecov.
|
|
The tests finally passed! There is no report to comparate against because we just introduced it :) So @Turbo87, ready for review! |
All of the actual mathematical tools can be used from the numpy module instead of the math module, and they will work transparently for arrays as well as floats. This version works both with and without numpy; and it should behave as before if numpy is not installed, or if it is installed and the inputs are floats. The only issues involved are related to bounds checking and other inequalities. Bounds checking is done with the utility function in_bounds which checks if ALL elements satisfy the conditions. For calculating the zone letters and numbers, only one element is used: the first one. It is assumed to be caller’s responsibility that all points lie in the same zone. Forcing the zone may be useful for numpy users, therefore added a force_zone_letter argument to from_latlon, to make the zone fully forceable. The only other caveat is that the south/north check in from_latlon may cause problems if the input latitude has mixed signs. This is therefore checked and may raise a ValueError. Tests do assume that numpy is installed, however.
|
Thanks @Juanlu001. I actually just pushed to your branch before I saw you close it, but we can use this one too. |
|
What has happened to this? It would be nice to crunch all of my values in one go. |
|
I think it's only missing a review by @Turbo87, the functionality and tests are there! |
|
@TheBB @Juanlu001 sorry, for taking so long. I don't use numpy and currently do very little Python myself, but I've just sent you an invite to the repo so you can help maintain the project if you want to. feel free to merge this PR if you think it's ready :) if you want to release a new version you only need to update the changelog, bump the version and push a tag to GitHub, and Travis CI will then take care of publishing the package to PyPI. |
|
Thanks for the invitation @Turbo87, I hope I will do a good job as a maintainer :) I will proceed and merge this, and hopefully we can prepare a release this week or the following one. |
|
(I restarted the build just in case, since it's been a while) |
|
And by "this week or the following one" I actually meant... Next one 🤞 |
5 participants
It was deemed “out of scope” in #18 but it's actually very simple to do and doubtless a useful feature. For people like me who are working with heightmaps with thousands of points in each direction in different coordinate systems, converting coordinate pairs one-by-one is unacceptably slow.
In principle, the numpy module implements all the functions that are needed from math, and they work transparently for arrays as well as floats, so it's a drop-in replacement.
Exceptions:
I wrote the tests to require numpy since it's safer. Not sure what Travis will think of this so let's see…
I'm already using this modified version in my code so it's not critical to me that this is merged, but I'm fairly sure this would be useful to others.