Skip to content

Add DL0 telescope event type, add missing types #2769

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Jun 6, 2025

Marking as draft for now, I will prepare a change request to the R1 model so that the additional types we define here are actually defined in the model and we don't risk that they are defined in an incompatible way in the future.

Also fixes #2094

@maxnoe maxnoe requested review from kosack and mdpunch June 6, 2025 16:27
Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 88.50% Coverage (94.20% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

@maxnoe
Copy link
Member Author

maxnoe commented Jun 6, 2025

Side note: the data model has event_type as a telescope-event level quantity, there is actually no concept of an event type for subarray events. I think this is still useful, but at least we need to add the event type at the telescope level here.

(#2094)

Copy link

@mdpunch mdpunch left a comment

Choose a reason for hiding this comment

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

Looks fine to me (though I haven't tried testing it), and nice to see it implemented for use downstream, thanks!

@mdpunch
Copy link

mdpunch commented Jun 7, 2025

Minor comment/question (not to do with the issue at hand): why does there need to be a special event type for LONG_EVENT, given that the shape of the waveform array will tell you this?
Asking since we may need some REDUCED_WINDOW_EVENT for GRBs (though that would be for the whole run).

@kosack
Copy link
Member

kosack commented Jun 12, 2025

Minor comment/question (not to do with the issue at hand): why does there need to be a special event type for LONG_EVENT, given that the shape of the waveform array will tell you this?
Asking since we may need some REDUCED_WINDOW_EVENT for GRBs (though that would be for the whole run).

That was requested by NectarCam in fact, but the idea of having it as a type is just to be able to keep statistics and monitor the rate of those events without having to read all the event data. It also may be that these long events are stored in a separate file (which is how we handle different calibration event types as well), which is more efficient since no variable-length arrays are needed. So if we also have some sort of "short" event that is random (not for the full run), we could consider generalizing this, or adding a type.

@mdpunch
Copy link

mdpunch commented Jun 12, 2025

That was requested by NectarCam in fact, but the idea of having it as a type is just to be able to keep statistics and monitor the rate of those events without having to read all the event data.

Minor comment/question (not to do with the issue at hand): why does there need to be a special event type for LONG_EVENT, given that the shape of the waveform array will tell you this?
Asking since we may need some REDUCED_WINDOW_EVENT for GRBs (though that would be for the whole run).

That was requested by NectarCam in fact, but the idea of having it as a type is just to be able to keep statistics and monitor the rate of those events without having to read all the event data. It also may be that these long events are stored in a separate file (which is how we handle different calibration event types as well), which is more efficient since no variable-length arrays are needed. So if we also have some sort of "short" event that is random (not for the full run), we could consider generalizing this, or adding a type.

Aha, thanks, I understand. So, the idea is for interspersed long events.

For a possible ReducedWindow run, I guess it would be with the standard event type, as long as the data handler can cope downstream with the window being different from the "usual" (though since the cameras for the different telescope sizes have different readout windows anyway, I expect that should be okay).

@maxnoe
Copy link
Member Author

maxnoe commented Jun 12, 2025

For a possible ReducedWindow run, I guess it would be with the standard event type, as long as the data handler can cope downstream with the window being different from the "usual" (though since the cameras for the different telescope sizes have different readout windows anyway, I expect that should be okay).

The CameraConfiguration has two fields, num_samples_nominal and num_samples_long. So if you have a whole run with reduced window, I would assume that num_samples_nominal is smaller already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update trigger event types to match R1 data model
3 participants