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

Create numba ufunc for sum of samples within charge extraction window #1038

Merged
merged 9 commits into from
Apr 24, 2019

Conversation

watsonjj
Copy link
Contributor

@watsonjj watsonjj commented Apr 3, 2019

This PR replaces the extract_charge_from_peakpos_array function with sum_samples_around_peakpos. This is a numba function that more efficiently extracts the charge from the waveform within the window defined by the peak position, window width, and window size.

It is defined using the @guvectorize decorator, which creates a numpy universal function, enabling the passing of both scalars and arrays for the peak position, window width, and window shift arguments.

The way the @guvectorize works (in this case) is that you define the operation that is applied for each channel and pixel. The operation is then optimised using numba's just-in-time compilation.

I have profiled sum_samples_around_peakpos against the previous extract_charge_from_peakpos_array function. The new function provides a factor of 60 reduction in execution time (from a couple of ms, to tens of µs).

It also provides a factor of 2 reduction in execution time upon the waveforms[:, :, start:end].sum(2) operation performed in GlobalPeakWindowSum.

The execution time for the extractors before this change were:

GlobalPeakWindowSum
6.15 ms ± 57.2 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
LocalPeakWindowSum
12 ms ± 209 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
NeighborPeakWindowSum
14.7 ms ± 293 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

The execution time for the extractors following this change are:

GlobalPeakWindowSum
3.26 ms ± 30.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
LocalPeakWindowSum
3.28 ms ± 28.3 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
NeighborPeakWindowSum
5.67 ms ± 33 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

The bottleneck is now the pulse_time calculation, which I could address in a different PR.

- Replace extract_charge_from_peakpos_array
@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@24707ab). Click here to learn what that means.
The diff coverage is 88.23%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1038   +/-   ##
========================================
  Coverage          ?   83.2%           
========================================
  Files             ?     186           
  Lines             ?   10583           
  Branches          ?       0           
========================================
  Hits              ?    8806           
  Misses            ?    1777           
  Partials          ?       0
Impacted Files Coverage Δ
ctapipe/image/tests/test_extractor.py 100% <100%> (ø)
ctapipe/image/extractor.py 67.67% <53.84%> (ø)

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 24707ab...cc3b326. Read the comment docs.

* master:
  Set out-of-bounds pulse time to -1
  Set to nan if out of range
  Rename UserWindowSum to FixedWindowSum
ctapipe/image/extractor.py Outdated Show resolved Hide resolved
kosack
kosack previously approved these changes Apr 15, 2019
@thomasarmstrong
Copy link
Contributor

I have had a look over the code and have run some tests to compare these changes to the extraction methods in the current master branch. Everything seems good to me, in fact this pull request also fixes a current bug in the GlobalPeakWindowSum which causes it to fail if there is only one channel. I would suggest that the PR is approved.

(One note, there is a typo in the sum_samples_around_peak description "The ret argument is required by numpy to creae the numpy array which is")

Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

Other than a typo looks good

ctapipe/image/extractor.py Outdated Show resolved Hide resolved
@watsonjj watsonjj dismissed stale reviews from thomasarmstrong and kosack via a8c207f April 23, 2019 13:13
maxnoe
maxnoe previously approved these changes Apr 23, 2019
* master:
  Correct function name
  Create extract_pulse_time_around_peakpos
  Fix passing config the CameraCalibrator
  Correct test
  Add test to show error in creating CameraCalibrator with config
  Correct min to max
  Fix test
  Update bokeh plotters to handle nan

# Conflicts:
#	ctapipe/image/extractor.py
@watsonjj watsonjj dismissed stale reviews from thomasarmstrong and maxnoe via 40cb749 April 23, 2019 14:43
* master:
  Fix See Also docs for sphinx 2 (cta-observatory#1051)
@watsonjj
Copy link
Contributor Author

@kosack @maxnoe Could you restore your review here

@kosack kosack merged commit 87273b3 into cta-observatory:master Apr 24, 2019
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request Apr 25, 2019
* master:
  Create numba ufunc for sum of samples within charge extraction window (cta-observatory#1038)
  Implement nan-handling like matplotlib high-level api (cta-observatory#1050)
  Fix See Also docs for sphinx 2 (cta-observatory#1051)
  Correct function name
  Create extract_pulse_time_around_peakpos
  Correct min to max
  Fix test
  Update bokeh plotters to handle nan
@watsonjj watsonjj deleted the sum_numba branch April 29, 2019 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants