search

HEXRD / hexrd

A cross-platform, open-source library for the analysis of X-ray diffraction data.
Other
56 stars 25 forks source link

Refactor rotations.py to use scipy, much faster and cleaner #688

Closed kevindlewis23 closed 1 month ago

kevindlewis23 commented 1 month ago

Rewrote a lot of the rotation math in rotations.py to call the scipy methods that do similar things. Benchamarking shows it's a lot faster and it cleans up the code a bit.

(I will definitely squash these changes before merging, there's so many because I was going back and forth between branches)

pep8speaks commented 1 month ago

Hello @kevindlewis23! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2024-07-23 13:44:42 UTC
codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 63.58382% with 63 lines in your changes missing coverage. Please review.

Project coverage is 31.33%. Comparing base (eb6f2fb) to head (5342057).

Files Patch % Lines
hexrd/rotations.py 63.58% 63 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #688 +/- ## ========================================== - Coverage 31.46% 31.33% -0.13% ========================================== Files 130 130 Lines 21900 21712 -188 ========================================== - Hits 6890 6804 -86 + Misses 15010 14908 -102 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kevindlewis23 commented 1 month ago

All of my actual functional changes were covered by tests by the way, what codecov is talking about is formatting changes from running Black, or the fact that I replaced the huge block of numpy function imports with just importing numpy, and then putting "np." in front of a bunch of thigngs. I decreased the percentage of coverage for the project because I shortened code that was already covered by my previous PR.

psavery commented 1 month ago

I can also update rotMatOfQuat to use scipy and it will be faster (I ran some benchmarking tests). However, if you run it multiple times with the same input, the current implementation is much faster because of the Numba LRU cache. Will this be run with the same rotation matrices a lot? If not, I can change it, it won't matter that much for speed because nothing super slow is happening

The numba caching is actually caching the compiled code, and is thus a better thing to test against. Users will most often be using the pre-compiled code. The first time they run the application, it will be slower due to compiling, but since they will not be modifying the code, they will always be using the pre-compiled code after that.

kevindlewis23 commented 1 month ago

The numba caching is actually caching the compiled code, and is thus a better thing to test against. Users will most often be using the pre-compiled code. The first time they run the application, it will be slower due to compiling, but since they will not be modifying the code, they will always be using the pre-compiled code after that.

Yeah thank you, I misunderstood what numba cache was. I removed that from my original comment