Add n saturated channels

[WenzDaniel]

What does the code in this PR do / what does it improve?

In this PR we do 3 things:

• Add n_saturated_channel information to peak_basics and propagate the information upwards to event_basics.
• Propagate the information about tight_coincidence up to event_basics
• Add a new tight_coincidence_channel field which contains the tight coincidence based on number of contributing PMTs instead of hits.

For the last change I modified get_tight_coincidence and I also updated the peak_merging accordingly in AxFoundation/strax#551. Further, I added a test for get_tight_coincidence which tests both the normal tight_coincidence and the channel based one.

Some of the test in this PR will currently, fail as we have to merge AxFoundation/strax#551 first and updated our peak-level test data.

Some disadvantage of this PR: We need to reprocess starting with peaklets if we want to include a channel based tight coincidence.

[skazama]

Thanks @WenzDaniel for your help on this, the code looks fine to me.

[JoranAngevaare]

Looks like a simple change, seems technically sound.

Like you say, it requires a reprocessing of peaklets and is breaking change as such.

The other thing I'd be cautious about is making event_basics ever expanding, this PR adds another 12 fields, perhaps it's good to explain in the PR description the envisaged use cases.

[WenzDaniel]

Motivating the fields:

• tight_coincidence and tight_coincidence_channel are required in almost any science analysis and hence I think it is good to give analysts the possibility to access those quantities easily without loading weeks of peak_basic data first. Which might either become challenging due to its data-size or tedious if loaded run by run and stored separately.
• For n_saturated channel it is kind of the same argument, but it was primarily requested by different analysts working on high energy and alpha decay studies. For this quantity it was even worse before as it was not even stored in peak_basics.

I think in general it is fine to add new quantities in event_basics if they are used by a multitude of different analyses. Storage-wise it does not add much. But it might save analysts dozens of hours time (in total across all analyses). I also do agree that it does not make sense to store every single quantity in the end.

Maybe also our ACs can comment about this.

[skazama]

I think in general it is fine to add new quantities in event_basics if they are used by a multitude of different analyses. Storage-wise it does not add much. But it might save analysts dozens of hours time (in total across all analyses). I also do agree that it does not make sense to store every single quantity in the end.

The proposed variables (especially tight_coincidence) are important for many studies, so I think it is fine to add these variables at the moment. We can later reduce the number of variables once all the studies are finalized.

[WenzDaniel]

Okay here is also the last missing piece. I just made a new PR with some updated test data which will be required if we want to merge this PR: XENONnT/strax_auxiliary_files#23.

[ershockley]

Thanks @WenzDaniel! This looks good to me after fixing the description for tight_coincidence_channel. I will approve after that.

I think we should eventually settle on a single definition for tight_coincidence, but for now it would be good to have both variables for direct comparisons.

[ershockley]

About the number of fields -- I think it is better to err on the side of too many columns over having missing information. I take @JoranAngevaare's point though, it is much easier to add columns than to remove them. I think these ones are important though.

[WenzDaniel]

Okay I fixed the description could you guys also approve: AxFoundation/strax#551 and XENONnT/strax_auxiliary_files#23. Then I can updated our tests in straxen with the new data. Unfortunately I cannot merge without approval as I am not an admin.

[JoranAngevaare]

@WenzDaniel , there shouldn't be a need for new test data, we only use the raw-records right?

[WenzDaniel]

No we also use 180215_1029-peaks in e.g. test_several I think. Let me quickly re-run then I can tell you which one it was :D

[JoranAngevaare]

No we also use 180215_1029-peaks in e.g. test_several I think. Let me quickly re-run then I can tell you which one it was :D

That's all 180423_1021

[WenzDaniel]

Just for the record I patched in the he channel support in 72f92d3.