Improve image combination performance

[mwcraig]

This pull request attempts to improve the performance of ccdproc by using using numpy's nan* functions instead of numpy MaskedArray, and using by using bottleneck. It is not intended (yet) to change the API for Combiner.

So far I have only switched average_combine to do this. If I get a couple 👍 on this I'll do the same for median_combine and the clipping routines, and also combine the implementations of sum_combine and average_combine to the extent possible.

To do:

• Document weighting during image combination
• Add link to preliminary benchmarking
• Unify sum and average combination
• Use nan* or bottleneck for clipping
• Changelog entry
• make bottleneck an optional dependency
• Add prominent suggestion to use bottleneck
• Consider making dtypes settable (might belong in CCDData). -- this is a separate issue, really, and CCDData lives in astropy, core, not here.

Edit: Fixes #719

[codecov]

Codecov Report

Merging #741 (3ef82d9) into main (bb9c667) will decrease coverage by 1.08%.
The diff coverage is 99.25%.

❗ Current head 3ef82d9 differs from pull request most recent head 50abe7c. Consider uploading reports for the commit 50abe7c to get more accurate results

@@            Coverage Diff             @@
##             main     #741      +/-   ##
==========================================
- Coverage   96.01%   94.93%   -1.09%     
==========================================
  Files          30       30              
  Lines        3942     4049     +107     
==========================================
+ Hits         3785     3844      +59     
- Misses        157      205      +48     

Impacted Files	Coverage Δ
ccdproc/combiner.py	94.50% <98.33%> (+0.47%)	⬆️
ccdproc/core.py	97.24% <100.00%> (ø)
ccdproc/tests/pytest_fixtures.py	86.66% <100.00%> (ø)
ccdproc/tests/test_combiner.py	100.00% <100.00%> (ø)
ccdproc/tests/test_memory_use.py	89.65% <100.00%> (+0.36%)	⬆️
ccdproc/tests/run_with_file_number_limit.py	24.21% <0.00%> (-49.48%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb9c667...50abe7c. Read the comment docs.

[mwcraig]

Performance improvements (no clipping done)

Some benchmark plots are below. left-most commit is master, right-most uses bottleneck. Second-to-last uses nan* instead of masked array.

average_combine with weighting: Speedup is ~ x2

Unweighted average_combine, some entries masked. Speedup is ~ x3

Unweigthed average_combine, no entries masked. Speedup is ~ x2.8

[mwcraig]

Ping @crawfordsm @MSeifert04 @saimn @cmccully @ysBach -- you are either a ccdproc maintainer or someone who is working hard separate from this to improve image combination speed. Keep an eye out for an email invite later today to talk about whether we can pool efforts on this or not.

I don't need a detailed review of this -- but if you have objects to this stop-gap approach for improving performance please speak up!

[saimn]

Great to see progress on this. I started to do some comparison with what we have in DRAGONS, but it is still a wip.

About bottleneck, since I had the opportunity to look at it more closely recently (astropy/astropy#10553 (comment)):

• It does have optimized algorithm only for little-endian data. For big-endian (FITS...) it has a fallback to np.nan* functions.
• And currently the fallback to numpy comes with a pretty bad memory leak.

Other than that, 👍 to use nan* functions instead of masked arrays, it is much faster.

[saimn]

Looking more in detail at the code it seems that ccdproc converts the data to np.float64 by default, unless a dtype is specified. So the good news is that it will not be affected by the bottleneck issue. But in terms of performance it would be faster to use the original data's dtype.

[mwcraig]

One other bottleneck note: there are some precision issues with sums of float32: pydata/bottleneck#193

[mwcraig]

@saimn @ysBach -- any chance either of you can take a look at this? It has become a little convoluted because I'm trying to make sure I don't change the API. THis si only a small step towards improving performance, but it is a step...

For median the only improve is if bottleneck is installed because np.nanmedian just calls np.ma.median after setting up a mask internally.

Once this is wrapped up (hopefully this week) I'd like to come back to further improvements, likely the second week of June.

[saimn]

Coming a bit late but I don't see any issue (and my knowledge of ccdproc's combine code is limited). 🎉