add numba_math.py; optimize coordinates.py and _small_angle_of_2_vectors - #1674
Conversation
E.g. locally seeing grid.construct_face_centers('welzl') time decrease from 14 s to ~150 ms, for a grid with 157k faces.
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
Benchmarks that have got worse:
|
|
Also, I think if we're going to go this route with |
|
Ope, wait, there's an issue with operator overloads. If you disable the numba jit, the decorator skips and Python goes back to concatenating with The joys of Python... |
|
Thank you @cmdupuis3 for reviewing!
Yeahhh, while Python objects would be happy to support a convenient interface with overloads like that, it seems to me like for performance-optimized numba it might be better to just keep the clunkier interface. Numba compilations make things harder to reason about, so keeping it simple seems to be the way to go here. (E.g. I also originally tried To me it doesn't feel like a change to the whole repo philosophy, only a change to the philosophy of numba-compiled functions (wherein this directly causes huge speedup). Outside of njit-decorated functions, I believe uxarray should certainly still use numpy/dask/xarray arrays for dealing with vectors! |
erogluorhan
left a comment
There was a problem hiding this comment.
This looks good to me, so I approve it; just a single comment below though.
…ttps://github.com/UXARRAY/uxarray into sevans/add-numba_math.py--optimize-coordinates.py
Closes #1680 and closes #1681 (both are sub-issues of #1648).
Overview
(For #1680) adds
uxarray/utils/numba_math.pyto provide numba routines primarily for 3-vectors. Applying these directly to tuples to avoid constructing lots of tiny numpy arrays inside numba routines can cause huge speedup in some cases.(For #1681) Optimizes numba routines in
uxarray/grid/coordinates.py, and the_small_angle_of_2_vectorsfunction (which is called directly by some routines in coordinates.py). As per discussion in #1648, this PR does not yet attempt to make these optimizations more broadly, but instead just focuses on improving a single area of the code.After these changes, locally I am seeing grid.construct_face_centers('welzl') time decrease from ~14 s to ~150 ms, for a grid with 157k faces. ASV benchmarks similarly show huge improvement (>=20x speedup) there.
Sidenote: attempted to make a cleverer / more convenient interface, such as
def _numba_add3(*vectors)which could add multiple vectors together, instead of justdef _numba_add3(u, v)but that leads to extremely confusing numba errors in case of any type mismatch between values in tuples. Seems like a better idea to keep this simple (yet maybe slightly less convenient) interface to avoid those confusing errors.Tiny expansion of PR scope: also fixes type-hinting for
_lonlat_rad_to_xyzand_xyz_to_lonlat_rad.PR Checklist
General
Testing & Benchmarking
Documentation
docs/api.rst_)AI Disclosure
AI Usage: GitHub Copilot's inline code suggestions; discussions with Claude Sonnet and ChatGPT to help understand and debug numba routines and confusing numba errors.