{"action":"created","issue":{"url":"https://api.github.com/repos/cta-observatory/ctapipe/issues/665","repository_url":"https://api.github.com/repos/cta-observatory/ctapipe","labels_url":"https://api.github.com/repos/cta-observatory/ctapipe/issues/665/labels{/name}","comments_url":"https://api.github.com/repos/cta-observatory/ctapipe/issues/665/comments","events_url":"https://api.github.com/repos/cta-observatory/ctapipe/issues/665/events","html_url":"https://github.com/cta-observatory/ctapipe/pull/665","id":302617352,"number":665,"title":"ctapipe.image.cleaning.tailcuts_clean speed up factor ~10","user":{"login":"dneise","id":8200858,"avatar_url":"https://avatars2.githubusercontent.com/u/8200858?v=4","gravatar_id":"","url":"https://api.github.com/users/dneise","html_url":"https://github.com/dneise","followers_url":"https://api.github.com/users/dneise/followers","following_url":"https://api.github.com/users/dneise/following{/other_user}","gists_url":"https://api.github.com/users/dneise/gists{/gist_id}","starred_url":"https://api.github.com/users/dneise/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dneise/subscriptions","organizations_url":"https://api.github.com/users/dneise/orgs","repos_url":"https://api.github.com/users/dneise/repos","events_url":"https://api.github.com/users/dneise/events{/privacy}","received_events_url":"https://api.github.com/users/dneise/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-03-06T09:09:05Z","updated_at":"2018-03-06T09:20:27Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/cta-observatory/ctapipe/pulls/665","html_url":"https://github.com/cta-observatory/ctapipe/pull/665","diff_url":"https://github.com/cta-observatory/ctapipe/pull/665.diff","patch_url":"https://github.com/cta-observatory/ctapipe/pull/665.patch"},"body":"We have a kind of default analysis in digicampipe, which uses `ctapipe.image.cleaning.tailcuts_clean`. I realized this part takes ~13% of the total runtime of the analysis, while it is not operating on the waveforms (n_pixel, n_samples) but on the image, which is a factor of 50x smaller than the waveforms. \r\n\r\nTypically runtime scales somehow with N ... so I was surprised this was so slow. I started digging and saw that tailcut_clean performs basically dot-products with the neighbor matrix. They are not really written as `numpy.dot`-products, but they essentially are dot-products.\r\n\r\nSince the neighbor-matrix is sparse by design these dots products can be sped by using that fact.\r\nsparsity of neighbor matrices:\r\n```\r\nName - fraction of non-zero entries:\r\n---------------------------------------------\r\nASTRICam 0.16%\r\nHESS-II 0.28%\r\nWhipple490 0.89%\r\nWhipple109 4.85%\r\nCHEC 0.19%\r\nFlashCam 0.33%\r\nWhipple151 3.58%\r\nWhipple331 1.70%\r\nDigiCam 0.45%\r\nHESS-I 0.60%\r\nLSTCam 0.31%\r\nVERITAS 1.14%\r\nNectarCam 0.31%\r\nSCTCam 0.03%\r\nFACT 0.40%\r\n```\r\n\r\nIn this PR I have just made a fast version of `tailcut_clean` alongside the normal version and implemented a few tests showing that both versions give still the same results and that the version, which claims to be faster is indeed faster.\r\n\r\nI found this call to show it quite nicely:\r\n```\r\n pytest ctapipe/image/tests/test_cleaning.py --durations=10\r\n```\r\n\r\nThe result on my machine is:\r\n```\r\ndneise@lair:~/cta/ctapipe$ pytest ctapipe/image/tests/test_cleaning.py --durations=10\r\n================================================= test session starts =================================================\r\nplatform linux -- Python 3.6.3, pytest-3.3.2, py-1.5.2, pluggy-0.6.0 -- /home/dneise/anaconda3/bin/python\r\ncachedir: .cache\r\nbenchmark: 3.1.1 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)\r\nrootdir: /home/dneise/cta/ctapipe, inifile: setup.cfg\r\nplugins: profiling-1.2.11, benchmark-3.1.1\r\ncollected 9 items                                                                                                     \r\n\r\nctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_simple PASSED                                         [ 11%]\r\nctapipe/image/tests/test_cleaning.py::test_dilate PASSED                                                        [ 22%]\r\nctapipe/image/tests/test_cleaning.py::test_tailcuts_clean PASSED                                                [ 33%]\r\nctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_min_neighbors_1 PASSED                                [ 44%]\r\nctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_min_neighbors_2 PASSED                                [ 55%]\r\nctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_with_isolated_pixels PASSED                           [ 66%]\r\nctapipe/image/tests/test_cleaning.py::test_tailcuts_fast_equal_to_normal PASSED                                 [ 77%]\r\nctapipe/image/tests/test_cleaning.py::test_1000_tailcuts PASSED                                                 [ 88%]\r\nctapipe/image/tests/test_cleaning.py::test_1000_tailcuts_fast PASSED                                            [100%]\r\n\r\n============================================== slowest 10 test durations ==============================================\r\n2.09s call     ctapipe/image/tests/test_cleaning.py::test_1000_tailcuts\r\n0.14s call     ctapipe/image/tests/test_cleaning.py::test_1000_tailcuts_fast\r\n0.05s call     ctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_simple\r\n0.04s call     ctapipe/image/tests/test_cleaning.py::test_tailcuts_fast_equal_to_normal\r\n0.03s call     ctapipe/image/tests/test_cleaning.py::test_dilate\r\n0.03s call     ctapipe/image/tests/test_cleaning.py::test_tailcuts_clean\r\n0.00s call     ctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_min_neighbors_1\r\n0.00s call     ctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_min_neighbors_2\r\n0.00s call     ctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_with_isolated_pixels\r\n0.00s setup    ctapipe/image/tests/test_cleaning.py::test_tailcuts_clean_simple\r\n============================================== 9 passed in 2.63 seconds ===============================================\r\n```\r\n\r\n## Things, I think are still to do:\r\n\r\n-----\r\n\r\n### Undo variable renaming:\r\n\r\nPlease note, I also renamed a couple of variable before I started to work, since I could not wrap my head around the meaning ... in hindsight this was stupid and I should rework this PR without the renaming. \r\n\r\n### Remove the duplication ...\r\n... of `tailcuts_clean_fast` and `tailcuts_clean` there should be only one version in the end. \r\n\r\n### Remove the tests...\r\n... showing that the fast version is indeed faster, since in the end there will probably be only the faster version.\r\n\r\n\r\n----\r\n\r\nGenerally I am interested to hear your opinion about performance improving PRs like this one? \r\nAre they already wanted? Or is this premature?\r\n\r\nAnd how do you like this style, where for a certain time both versions co-exist to test and compare them against each other and then after some time we decide to drop one version... is this the right way to go?\r\n\r\nLooking forward to your reviews."},"comment":{"url":"https://api.github.com/repos/cta-observatory/ctapipe/issues/comments/370716113","html_url":"https://github.com/cta-observatory/ctapipe/pull/665#issuecomment-370716113","issue_url":"https://api.github.com/repos/cta-observatory/ctapipe/issues/665","id":370716113,"user":{"login":"codecov[bot]","id":22429695,"avatar_url":"https://avatars2.githubusercontent.com/in/254?v=4","gravatar_id":"","url":"https://api.github.com/users/codecov%5Bbot%5D","html_url":"https://github.com/apps/codecov","followers_url":"https://api.github.com/users/codecov%5Bbot%5D/followers","following_url":"https://api.github.com/users/codecov%5Bbot%5D/following{/other_user}","gists_url":"https://api.github.com/users/codecov%5Bbot%5D/gists{/gist_id}","starred_url":"https://api.github.com/users/codecov%5Bbot%5D/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/codecov%5Bbot%5D/subscriptions","organizations_url":"https://api.github.com/users/codecov%5Bbot%5D/orgs","repos_url":"https://api.github.com/users/codecov%5Bbot%5D/repos","events_url":"https://api.github.com/users/codecov%5Bbot%5D/events{/privacy}","received_events_url":"https://api.github.com/users/codecov%5Bbot%5D/received_events","type":"Bot","site_admin":false},"created_at":"2018-03-06T09:20:27Z","updated_at":"2018-03-06T09:20:27Z","author_association":"NONE","body":"# [Codecov](https://codecov.io/gh/cta-observatory/ctapipe/pull/665?src=pr&el=h1) Report\n> Merging [#665](https://codecov.io/gh/cta-observatory/ctapipe/pull/665?src=pr&el=desc) into [master](https://codecov.io/gh/cta-observatory/ctapipe/commit/a00df3b79b9c831a90f5982f1036598b26cc633a?src=pr&el=desc) will **increase** coverage by `0.15%`.\n> The diff coverage is `100%`.\n\n[![Impacted file tree graph](https://codecov.io/gh/cta-observatory/ctapipe/pull/665/graphs/tree.svg?width=650&height=150&src=pr&token=BP1Oq37DGJ)](https://codecov.io/gh/cta-observatory/ctapipe/pull/665?src=pr&el=tree)\n\n```diff\n@@            Coverage Diff             @@\n##           master     #665      +/-   ##\n==========================================\n+ Coverage    66.8%   66.96%   +0.15%     \n==========================================\n  Files         189      189              \n  Lines       10063    10110      +47     \n==========================================\n+ Hits         6723     6770      +47     \n  Misses       3340     3340\n```\n\n\n| [Impacted Files](https://codecov.io/gh/cta-observatory/ctapipe/pull/665?src=pr&el=tree) | Coverage Δ | |\n|---|---|---|\n| [ctapipe/image/tests/test\\_cleaning.py](https://codecov.io/gh/cta-observatory/ctapipe/pull/665/diff?src=pr&el=tree#diff-Y3RhcGlwZS9pbWFnZS90ZXN0cy90ZXN0X2NsZWFuaW5nLnB5) | `100% <100%> (ø)` | :arrow_up: |\n| [ctapipe/instrument/camera.py](https://codecov.io/gh/cta-observatory/ctapipe/pull/665/diff?src=pr&el=tree#diff-Y3RhcGlwZS9pbnN0cnVtZW50L2NhbWVyYS5weQ==) | `96.47% <100%> (+0.07%)` | :arrow_up: |\n| [ctapipe/image/cleaning.py](https://codecov.io/gh/cta-observatory/ctapipe/pull/665/diff?src=pr&el=tree#diff-Y3RhcGlwZS9pbWFnZS9jbGVhbmluZy5weQ==) | `100% <100%> (ø)` | :arrow_up: |\n\n------\n\n[Continue to review full report at Codecov](https://codecov.io/gh/cta-observatory/ctapipe/pull/665?src=pr&el=continue).\n> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)\n> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`\n> Powered by [Codecov](https://codecov.io/gh/cta-observatory/ctapipe/pull/665?src=pr&el=footer). Last update [a00df3b...2523d38](https://codecov.io/gh/cta-observatory/ctapipe/pull/665?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).\n"}}
{"actor":{"display_login":"codecov"}}