-
Notifications
You must be signed in to change notification settings - Fork 269
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
cep 2: event structure #2304
cep 2: event structure #2304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be useful to mention what this change would affect: namely all code that uses ctapipe's event-level API would need to be updated. Perhaps giving an example (or a few common examples) of what needs to be done to upgrade that software, e.g.:
# The orignal:
event.dl0.tel[15].waveform
# becomes
event.tel[15].dl0.waveform
Maybe also an example of how loops are simplified.
In other words, maybe add a "Migration Guide" section to help e.g. NectarChain and LSTChain
Looking at the proposed structure the muon information is missing. In the current stucture it is listed on the same structure-level as the data-levels. What would be the new structure for it? |
I have a few comment/questions:
My uneducated guess would tend to prefer the following structure: Meaning that the array of event would be directly packed as data level then telescope when it make sense, or for dl3, only have I don't know if from a code point of view this make sense, but that's how I'd do it. |
Yes, we probably won't have
That's what we have now and this CEP is proposing to change that. I will expand the text to better explain, but I think it's the opposite: we will have DL0 event files per telescope, so we might want to process DL0 to DL1 for telescopes individually, which will be much more natural if we have multiple data levels inside one It is also much more natural then to merge multiple By the way: this is also how sim_telarray output is organized:
|
Muon info are just DL1 parameters, so would appear under |
I updated the cep with a couple of before / after code examples and some more text |
dfb5912
to
62151db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with the new examples added.
One question is when to involve other stakeholders, i.e. the LST, NectarCam, CHEC/SSTCam, (and maybe MAGIC?) teams that are using ctapipe already. Should we ask for comments or just go ahead? At the very least, we might want to demo it at a weekly meeting and invite them to join |
Oh, one thing I just realized is not mentioned here that could be useful is the canonical way to determine if a telescope has information at a particular data level. Now that there is only one tel_id map, just looking at In the example: hillas_dicts = {
tel_id: dl1.parameters.hillas
for tel_id, dl1 in event.dl1.items()
}
#After:
hillas_dicts = {
tel_id: tel_event.dl1.parameters.hillas
for tel_id, tel_event in event.tel.items()
} the case where there is no |
How we work at the moment is that all telescopes get all data levels at the same time and we fill invalid values for telescopes that do not fulfill some criteria (e.g. we will have the image parameters, but they might be nan if not enough pixels survived the cleaning). So the situation you mentioned, that there are dl1 containers for some telescopes but not others cannot really happen. The DL1 container will be there for all telescope events, but some might contain invalid value markers. The full loop in the hillas reconstructors looks like this now:
and will look like this after the change:
|
To restate what I said in inline comment (in violation of CEP 1!), I think this CEP looks pretty clear, and the reasons for carrying out these changes are sound.
I just re-read CEP 1, and one thing I think we've forgotten is announcing this CEP according to the workflow we defined, in particular I think this haven't happened:
so some sort of announcement that will reach more people is already mandated. Separately, I think prodding those "external" stakeholders to leave a review (or at least proof -of-skimming) is valuable, but I'll let @kosack judge if a mailing list announcement is a firm enough poke to get some response. |
I made one last change (removing This is related to #2374, trigger info should be part of DL0. |
I started implementing a zfits event source following the current draft of the ACADA DPPS ICD, in part as preparation for the ACADA LST integration tests on La Palma. On thing I realized is that with this proposal here, it would be possible to implement subarray event sources and telescope event sources. The latter would be useful for reading DL0 telescope event data and processing it up to dl1 in parallel, where we don't need stereo information and to read the anticipated separate streams for shower, calibration and muon events. E.g. calib pipe would probably only ever use a A possible |
Will merge into "proposed" and we will open a new PR moving it to "Accepted" for external review |
5590e29
to
5e00773
Compare
Ok, merging now to have it under proposed rendered in the docs so we can collect comments. |
No description provided.