Skip to content

Conversation

@psavery
Copy link
Collaborator

@psavery psavery commented Jun 16, 2025

Profiling showed that the list comprehension in objFuncFitGrain was taking a significant amount of time. This is because that function is called repeatedly during the least squares fit, and repeatedly performing the list comprehension would take a substantial amount of time.

Performing the list comprehension outside of objFuncFitGrain was shown to provide a substantial speedup.

This is something I discovered when I was working on the spot tracking code. I'm simply cherry-picking that commit here.

This only affects the fitting part, and not the pull_spots() part.

Profiling showed that the list comprehension in `objFuncFitGrain`
was taking a significant amount of time. This is because that
function is called repeatedly during the least squares fit, and
repeatedly performing the list comprehension would take a substantial
amount of time.

Performing the list comprehension outside of `objFuncFitGrain` was
shown to provide a substantial speedup.

Signed-off-by: Patrick Avery <[email protected]>
@codecov
Copy link

codecov bot commented Jun 16, 2025

Codecov Report

Attention: Patch coverage is 72.54902% with 14 lines in your changes missing coverage. Please review.

Project coverage is 48.66%. Comparing base (10a6691) to head (49851c7).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
hexrd/fitting/grains.py 75.00% 4 Missing ⚠️
hexrd/imageseries/process.py 70.00% 3 Missing ⚠️
hexrd/imageseries/load/framecache.py 88.88% 2 Missing ⚠️
hexrd/imageseries/load/imagefiles.py 0.00% 2 Missing ⚠️
hexrd/imageseries/load/rawimage.py 0.00% 2 Missing ⚠️
hexrd/imageseries/load/hdf5.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #816      +/-   ##
==========================================
+ Coverage   48.61%   48.66%   +0.04%     
==========================================
  Files         143      143              
  Lines       22825    22870      +45     
==========================================
+ Hits        11097    11129      +32     
- Misses      11728    11741      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@psavery
Copy link
Collaborator Author

psavery commented Jun 16, 2025

The second commit on this PR adds support for indexing into an imageseries like this: ome_imgser[i_frame, ijs[0], ijs[1]], instead of the previous method of ome_imgser[i_frame][ijs[0], ijs[1]].

This means that instead of creating a whole dense array, and then performing fancy indexing into it, we can perform fancy indexing on the sparse array directly, and then create the array from that. This has the potential to speed up pull_spots() significantly. For one case I just studied recently, it sped up pull_spots() by around 5x.

Rather than expanding the full csr_matrix into a numpy array, then
performing fancy indexing on that, do the reverse: perform the
fancy indexing on the csr_matrix, and then expand it into a numpy
array.

This has the potential to save huge amounts of computing time, since
we do not have to create the entire dense array before performing
the indexing.

For the multiruby test case, this did not show any change to the
performance. However, for a monolithic Eiger example from Josh Ward,
it showed a 5x speed improvement.

Signed-off-by: Patrick Avery <[email protected]>
psavery added 2 commits June 16, 2025 16:24
This extracts the image data from the sparse array even faster,
because it skips the creation of the unnecessary extra `csr_matrix()`
(and its subsequent conversion back to a numpy array).

Signed-off-by: Patrick Avery <[email protected]>
@psavery psavery requested a review from bnmajor June 16, 2025 23:55
@psavery psavery merged commit 20bb998 into master Jun 17, 2025
7 checks passed
@psavery psavery deleted the fit-grains-perf branch June 17, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants