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

PERF: Optimize for single point in Geod fwd/inv functions #1206

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

greglucas
Copy link
Contributor

This adds a fast-path for scalar inputs by trying out a double-only implementation, and falling back if any input is not a double. An alternative is to add Geod.fwd_point() and Geod.inv_point() functions as well, but I kind of like this transparent form for simplicity of users only needing one function and only a minor overhead for arrays where you are already going through some slower paths.

A little under 10x improvement for scalars.

In [1]: from pyproj import Geod

In [2]: g = Geod(ellps="WGS84")

In [3]: %timeit g.fwd([0], [0], [1], [1])
4.76 µs ± 28.5 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [4]: %timeit g.fwd(0, 0, 1, 1)
588 ns ± 4.57 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
  • Tests added

@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #1206 (b752dd9) into main (1e309c2) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b752dd9 differs from pull request most recent head b93fedf. Consider uploading reports for the commit b93fedf to get more accurate results

@@            Coverage Diff             @@
##             main    #1206      +/-   ##
==========================================
- Coverage   96.28%   96.27%   -0.01%     
==========================================
  Files          20       20              
  Lines        1803     1801       -2     
==========================================
- Hits         1736     1734       -2     
  Misses         67       67              
Impacted Files Coverage Δ
pyproj/geod.py 97.54% <100.00%> (-0.03%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@snowman2
Copy link
Member

Nice! I am starting to think that following this pattern in #1204 may make sense as well.

@greglucas
Copy link
Contributor Author

I think it would be nice to add this pattern to the transform() call as well in #1204. Then downstream users wouldn't need to call a separate function at all and their code would benefit immediately. In addition though, since there is a separate PROJ function for this, do you want to still expose it as transform_point() or just leave that as an implementation detail within transform()?

@snowman2 snowman2 added this to the 3.5.0 milestone Dec 19, 2022
pyproj/geod.py Outdated Show resolved Hide resolved
pyproj/_geod.pyx Outdated Show resolved Hide resolved
pyproj/_geod.pyx Outdated Show resolved Hide resolved
@snowman2 snowman2 changed the title ENH: Add single point optimized Geod fwd/inv functions PERF: Optimize for single point in Geod fwd/inv functions Dec 20, 2022
This adds a fast-path for scalar inputs and falls back to
the previous implementation if any inputs can't be cast to double.
@snowman2
Copy link
Member

snowman2 commented Dec 21, 2022

My main hesitation to merge is the amount of code added versus the amount of code tested. The tests you added are a good start. However, I am thinking it may be a good idea to update the fwd/inv geodesic tests with the scalar_and_array fixture before merging this.

@snowman2 snowman2 merged commit 6f7a406 into pyproj4:main Dec 21, 2022
@snowman2
Copy link
Member

Thanks @greglucas 👍

@greglucas greglucas deleted the geod-singlepoint branch December 21, 2022 14:04
@greglucas
Copy link
Contributor Author

👍 Thanks, @snowman2!

FYI that this all works really nicely over on Cartopy without any update needed to the code at all. The benchmark case I was running went from 7s on pyproj==3.4 to 1.1s on pyproj==main so roughly 6-7x improvement!

@snowman2
Copy link
Member

That's great 🎉

@dopplershift
Copy link
Contributor

Thanks for doing all this @greglucas !

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.

3 participants