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.

Sign up for GitHub

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

Jump to bottom

Use sparse neighbor matrix #826

Merged
merged 2 commits into from
Dec 12, 2018
Merged

Use sparse neighbor matrix #826

merged 2 commits into from
Dec 12, 2018

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Nov 20, 2018 •
edited
Loading

Speed up of ~7

@codecov
Copy link

codecov bot commented Nov 20, 2018 •
edited
Loading

Codecov Report

Merging #826 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #826   +/-   ##
=======================================
  Coverage   73.52%   73.52%           
=======================================
  Files         211      211           
  Lines       12066    12066           
=======================================
  Hits         8871     8871           
  Misses       3195     3195
Impacted Files Coverage Δ
ctapipe/image/cleaning.py 100% <100%> (ø) ⬆️

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 e715248...7985df0. Read the comment docs.

@dneise
Copy link
Member

dneise commented Nov 26, 2018

changing one line and getting a speedup of ~7 ? nice ...
Could you comment on this a bit? Or is there an issue explaining what is going on?

@kosack
Copy link
Contributor

kosack commented Nov 28, 2018

should we just change CameraGeom.neighbor_matrix to be a sparse matrix by default? Is there any code left that uses the full matrix? Even if so, it can be cast to an array to un-sparisfy the matrix. Then we can get rid of the sparse_matrix attribute altogether.

@kosack
Copy link
Contributor

kosack commented Nov 28, 2018

changing one line and getting a speedup of ~7 ? nice ...
Could you comment on this a bit? Or is there an issue explaining what is going on?

I think it's just that the operation is O(n^2) where n is the number of pixels if you have a full matrix, but since the matrix is really sparsely filled since there are only at most 7 neighbors of a pixel (so a row in the matrix will only have up to 7 non-zero entries), any operation that works on the sparse version will be something closer to O(n).

Sparse matrices are stored in such a way that they only store filled entries.

IT's not totally clear that this speedup is always true, since in most cases doing operations on aligned data structures can be optimized by the CPU and the sparse version may pose some problems for that, but clearly numpy doesn't do that optimization at this point.

@maxnoe
Copy link
Member Author

maxnoe commented Nov 28, 2018

Could you comment on this a bit? Or is there an issue explaining what is going on?

The scipy function we uses always works on sparse matrices, if the input is not sparse, it is converted to a sparse matrix. We already have it, so the conversion is avoided.

@dneise
Copy link
Member

dneise commented Nov 28, 2018

Thx Max

@kbruegge kbruegge merged commit f9e91c5 into master Dec 12, 2018
@kbruegge kbruegge deleted the use_sparse_matrix branch December 12, 2018 09:10
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request Jan 7, 2019
@watsonjj
Merge branch 'master' into charge_resolution
92f9866 
* master: (22 commits)
  Remove all hillas_paramters but version 5, fixes cta-observatory#866  (cta-observatory#904)
  Fix docstring of EventSeeker, fixes cta-observatory#768 (cta-observatory#914)
  Do not set autodownload, fixes doc build (cta-observatory#910)
  Import bokeh.palletes correctly, fixes cta-observatory#911 (cta-observatory#912)
  Fix SST-1M to be DC and not SC. Solves cta-observatory#905 (cta-observatory#908)
  Fix a few test warnings (cta-observatory#902)
  Updates of NectarCam and LSTCam real data eventsource class (cta-observatory#812)
  Implemented FACT image cleaning (cta-observatory#862)
  remove `config=None, tool=None` in some tests (cta-observatory#897)
  Remove flow submodule (got moved to its own package) (cta-observatory#893)
  Cleaning the ctapipe folder. this has been empty for 3 years. (cta-observatory#892)
  Additional metadata from pyhessio, similar to cta-observatory#655 (cta-observatory#895)
  add scikit-learn to install_requires (cta-observatory#877)
  Add .mailmap (cta-observatory#894)
  Fix subarray peek. No more warnings. (cta-observatory#841)
  SimTelEventSource using pyeventio (cta-observatory#864)
  Use sparse neighbor matrix (cta-observatory#826)
  Add test how it should be (cta-observatory#842)
  fix errordef > 0. (cta-observatory#839)
  Fix warning about already closed hessio file (cta-observatory#834)
  ...

# Conflicts:
#	ctapipe/analysis/camera/chargeresolution.py
#	ctapipe/tools/extract_charge_resolution.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kosack kosack kosack approved these changes

@kbruegge kbruegge kbruegge approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maxnoe @dneise @kosack @kbruegge