Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use sparse matrix approach for neighbor window sum #1398

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Jul 22, 2020

Use same neighbor approach like number_of_islands

This results in a speedup from:

166.10 ± 66.14 µs

to

69.12 ± 4.01 µs

using:

import numpy as np
from ctapipe.instrument import TelescopeDescription, SubarrayDescription
import astropy.units as u
from ctapipe.image.extractor import NeighborPeakWindowSum
from timeit import repeat


tel = TelescopeDescription.from_name('LST', 'LSTCam')
geom = tel.camera.geometry
subarray = SubarrayDescription(
    'perf',
    tel_positions={1: [0, 0, 0] * u.m},
    tel_descriptions={1: tel},
)


extractor = NeighborPeakWindowSum(subarray)

waveforms = np.random.normal(size=geom.n_pixels).astype('float32')
selected_gain_channels = np.zeros(geom.n_pixels, dtype=int)


# number_of_islands(geom, mask)
code = '''
extractor(waveforms, 1, selected_gain_channels)
'''

exec(code)

N = 200
times = np.array(repeat(code, number=N, repeat=10, globals=globals()))
print(f'{np.mean(times / N) * 1e6:.2f} ± {np.std(times / N) * 1e6:.2f} µs')

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #1398 into master will decrease coverage by 0.04%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1398      +/-   ##
==========================================
- Coverage   90.90%   90.86%   -0.05%     
==========================================
  Files         184      184              
  Lines       12715    12735      +20     
==========================================
+ Hits        11559    11572      +13     
- Misses       1156     1163       +7     
Impacted Files Coverage Δ
ctapipe/instrument/camera/geometry.py 90.24% <ø> (-0.16%) ⬇️
ctapipe/tools/tests/test_tools.py 97.40% <ø> (-0.04%) ⬇️
ctapipe/image/extractor.py 85.37% <14.28%> (-1.23%) ⬇️
ctapipe/image/muon/intensity_fitter.py 90.54% <20.00%> (-2.47%) ⬇️
ctapipe/containers.py 100.00% <100.00%> (ø)
ctapipe/image/tests/test_extractor.py 100.00% <100.00%> (ø)
ctapipe/io/simteleventsource.py 94.64% <100.00%> (ø)
ctapipe/tools/stage1.py 85.77% <100.00%> (-0.06%) ⬇️
ctapipe/core/tests/test_component.py 97.71% <0.00%> (+0.16%) ⬆️
ctapipe/core/component.py 96.72% <0.00%> (+0.23%) ⬆️

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 597c1dc...834097c. Read the comment docs.

watsonjj
watsonjj previously approved these changes Jul 22, 2020
Copy link
Contributor

@watsonjj watsonjj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. From trying this PR it all seems to be working correctly and 2x faster than before

@maxnoe
Copy link
Member Author

maxnoe commented Jul 23, 2020

Side point: numba jitted functions do not contribute to coverage, we might want to create one instance of the tests with NUMBA_DISABLE_JIT=1 for the coverage report.

@kosack kosack merged commit eb571cf into master Jul 23, 2020
@maxnoe maxnoe deleted the neighbor_window_speedup branch July 23, 2020 13:57
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.

3 participants