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

Stage1 performance regression #1791

Closed
maxnoe opened this issue Sep 21, 2021 · 2 comments · Fixed by #1792
Closed

Stage1 performance regression #1791

maxnoe opened this issue Sep 21, 2021 · 2 comments · Fixed by #1792
Milestone
v0.12.0

Comments

@maxnoe
Copy link
Member

maxnoe commented Sep 21, 2021

0.10.5:

❯ ctapipe-stage1 --input ~/.cache/ctapipe/cccta-dataserver.in2p3.fr/data/ctapipe-extra/v0.3.3/gamma_test_large.simtel.gz --progress --overwrite --output=test2.dl1.h5
SimTelEventSource: 110ev [00:12,  8.70ev/s]

0.11.0:

❯ ctapipe-stage1 --input ~/.cache/ctapipe/cccta-dataserver.in2p3.fr/data/ctapipe-extra/v0.3.3/gamma_test_large.simtel.gz --progress --overwrite --output=test2.dl1.h5
SimTelEventSource: 0ev [00:00, ?ev/s]2021-09-21 10:34:43,142 WARNING [ctapipe.ctapipe-stage1.DL1Writer] (dl1writer._setup_output_path): Overwriting /home/maxnoe/Uni/CTA/ctapipe/test2.dl1.h5
SimTelEventSource: 110ev [00:07, 15.09ev/s]

Current master:

❯ ctapipe-process --input ~/.cache/ctapipe/cccta-dataserver.in2p3.fr/data/ctapipe-extra/v0.3.3/gamma_test_large.simtel.gz --progress --overwrite --output=test2.dl1.h5
2021-09-21 10:35:41,324 WARNING [ctapipe.io.datawriter.DataWriter] (datawriter._setup_output_path): Overwriting /home/maxnoe/Uni/CTA/ctapipe/test2.dl1.h5
SimTelEventSource: 110ev [00:50,  2.17ev/s]
@maxnoe maxnoe added this to the v0.12.0 milestone Sep 21, 2021
@maxnoe
Copy link
Member Author

maxnoe commented Sep 21, 2021

Most time is spent in calc_pixel_neighbors, so the reason is that each telescope has it's own instance of the camera geometry now. This means we basically don't have a "per event" regression but a larger overhead upfront.

The situation is worsed for very large arrays like the one in gamma_test_large.simtel.gz.

I'll see if I can improve the performance of calc_pixel_neighbors, I'd rather not go back to shared instances

maxnoe added a commit that referenced this issue Sep 21, 2021
@maxnoe
Improve performance of calc_pixel_neighbors, fixes #1791
559aa42 
All operations are now vectorized over all pixels and the csr
matrix is directly created instead of going through a lil
representation.

This brings stage1 performance on gamma_test_large.simtel.gz back to
where it was before changing to one CameraGeometry instance per
telescope.
@maxnoe maxnoe linked a pull request Sep 21, 2021 that will close this issue
Improve performance of calc_pixel_neighbors, fixes #1791 #1792
Merged
@maxnoe
Copy link
Member Author

maxnoe commented Sep 21, 2021

With #1792, we are back to:

❯ ctapipe-process --input ~/.cache/ctapipe/cccta-dataserver.in2p3.fr/data/ctapipe-extra/v0.3.3/gamma_test_large.simtel.gz --progress --overwrite --output=test2.dl1.h5
2021-09-21 11:15:03,464 WARNING [ctapipe.io.datawriter.DataWriter] (datawriter._setup_output_path): Overwriting /home/maxnoe/Uni/CTA/ctapipe/test2.dl1.h5
SimTelEventSource: 110ev [00:08, 13.52ev/s]

maxnoe added a commit that referenced this issue Sep 21, 2021
@maxnoe
Improve performance of calc_pixel_neighbors, fixes #1791
252f76e 
All operations are now vectorized over all pixels and the csr
matrix is directly created instead of going through a lil
representation.

This brings stage1 performance on gamma_test_large.simtel.gz back to
where it was before changing to one CameraGeometry instance per
telescope.
maxnoe added a commit that referenced this issue Sep 21, 2021
@maxnoe
Merge pull request #1792 from cta-observatory/calc_neighbor_matrix_ve…
e632369 
…ctorized

Improve performance of calc_pixel_neighbors, fixes #1791
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging a pull request may close this issue.

Improve performance of calc_pixel_neighbors, fixes #1791 cta-observatory/ctapipe
1 participant
@maxnoe