pysal / momepy

Urban Morphology Measuring Toolkit
https://docs.momepy.org
BSD 3-Clause "New" or "Revised" License
496 stars 59 forks source link

REF: speed-up describe using numba #571

Closed martinfleis closed 7 months ago

martinfleis commented 7 months ago

Follow-up on #570

Had to reimplement mode to actually compile based on https://github.com/numba/numba/pull/2959#issuecomment-741715733.

Timings compared to #570

only quantiles
# new 2.23 s ± 65.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
# old 7.96 s ± 59.5 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

no quantiles, mode
# new 1.25 s ± 7.76 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
# old 15 s ± 488 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

quantiles and mode
# new 2.48 s ± 42.9 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
# old  23 s ± 562 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

How would you set the CI envs to test numba and fallback options? I am not a big fan of having everything twice. Would 1 env without numba be enough in your experience?

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 96.29630% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.7%. Comparing base (4037c70) to head (5b75742). Report is 16 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/pysal/momepy/pull/571/graphs/tree.svg?width=650&height=150&src=pr&token=VNn0WR5JWT&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pysal)](https://app.codecov.io/gh/pysal/momepy/pull/571?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pysal) ```diff @@ Coverage Diff @@ ## main #571 +/- ## ======================================= + Coverage 97.4% 97.7% +0.4% ======================================= Files 26 35 +9 Lines 4328 5099 +771 ======================================= + Hits 4214 4984 +770 - Misses 114 115 +1 ``` | [Files](https://app.codecov.io/gh/pysal/momepy/pull/571?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pysal) | Coverage Δ | | |---|---|---| | [momepy/functional/\_diversity.py](https://app.codecov.io/gh/pysal/momepy/pull/571?src=pr&el=tree&filepath=momepy%2Ffunctional%2F_diversity.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pysal#diff-bW9tZXB5L2Z1bmN0aW9uYWwvX2RpdmVyc2l0eS5weQ==) | `98.0% <96.3%> (ø)` | |
jGaboardi commented 7 months ago

How would you set the CI envs to test numba and fallback options? I am not a big fan of having everything twice. Would 1 env without numba be enough in your experience?

Yes, I think we can add a note to the README, docs, and wherever else that we strongly encourage installing & using numba. Then perhaps adding a 312-min.yml to the matrix that doesn't include numba.