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

HDF5EventSource.__len__ is wrong if allowed_tels is given #2324

Open
maxnoe opened this issue May 9, 2023 · 2 comments
Open

HDF5EventSource.__len__ is wrong if allowed_tels is given #2324

maxnoe opened this issue May 9, 2023 · 2 comments
Labels

Comments

@maxnoe
Copy link
Member

maxnoe commented May 9, 2023

Describe the bug

When allowed_tels is set for HDF5EventSource, events with no telescopes remaining in the selection are skipped, thus changing the total number of array events.

Currently, __len__ simply looks at the number of array events in the input table, thus resulting in a too large number if allowed_tels is given.

To Reproduce

Steps to reproduce the behavior:

Run ctapipe-process -i dataset://gamma_prod5.simtel.zst -o /tmp/test.dl1.h5

❯ ctapipe-process -i dataset://gamma_prod5.simtel.zst -o /tmp/test.dl1.h5 --progress
2023-05-09 17:33:13,462 WARNING [ctapipe.core.telescope_component] (telescope_component.attach_subarray): TelescopeParameter type argument 'SST_1M_*' did not match any known telescope types
SimTelEventSource: 7ev [00:06,  1.07ev/s]
❯ ctapipe-process -i /tmp/test.dl1.h5 -o /tmp/test2.dl1.h5 --EventSource.allowed_tels=1 --EventSource.allowed_tels=2 --EventSource.allowed_tels=3 --EventSource.allowed_tels=4 --progress
2023-05-09 17:34:38,508 WARNING [ctapipe.core.telescope_component] (telescope_component.attach_subarray): TelescopeParameter type argument 'MST_*' did not match any known telescope types
2023-05-09 17:34:38,508 WARNING [ctapipe.core.telescope_component] (telescope_component.attach_subarray): TelescopeParameter type argument 'SST_1M_*' did not match any known telescope types
HDF5EventSource:  14%|███████▋                                              | 1/7 [00:01<00:09,  1.56s/ev]

Observe that the progress bar thinks there will be 7 events but only 1 is actually encountered.

Expected behavior
__len__ provides the correct length

@maxnoe maxnoe added the bug label May 9, 2023
@maxnoe
Copy link
Member Author

maxnoe commented May 9, 2023

There are I think three possible solutions:

  1. Keep the current behavior of the source but remove __len__, making it a pure iterator with an unknown a-priori length
  2. Keep the current, simple length implementation but yield also "empty" events (that could then be taken care of by the Softwaretrigger)
  3. Implement a more complex __len__ that looks at the full trigger table if allowed_tels are given

@kosack
Copy link
Contributor

kosack commented May 11, 2023

I like having a __len__ attribute, even if it is not totally reliable, as it still gives useful information. So I'd prefer either keeping it the way it is (in which case the progress bar is an upper limit, I guess, but still somewhat useful), or implement suggestion 2 or 3.

@maxnoe maxnoe added the IO label Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants