XENONnT / straxen

Streaming analysis for XENON
BSD 3-Clause "New" or "Revised" License
20 stars 32 forks source link

fix peak per event plugin #1400

Closed RoiFrankel closed 2 weeks ago

RoiFrankel commented 2 months ago

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

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

Fix the Bug of the mismatch between peaks['event_number'] and event['event_number'] given by peak_per_event plugin. More details on the bug and his solution are at the following wiki note: https://xe1t-wiki.lngs.infn.it/doku.php?id=xenon:xenonnt:analysis:analysts_overview_page:roi_frankel:Event_number_bug

Can you briefly describe how it works?

We start by sorting the indices of fully contained peaks. Then, we create a dictionary called 'mapping', where keys represent event numbers in peak level data, and values represent event numbers in event level data.

The 'mapping' dictionary ensures that each event number at the peak level corresponds to the correct event number at the event level. We then use 'corrected_split_peaks_ind' to update the event numbers of peaks based on the 'mapping' dictionary. This process ensures that event numbers match between peaks and events. The 'result['event_number']' array is updated with 'corrected_split_peaks_ind', ensuring that event numbers in peaks are consistent with those in events, maintaining the original length and preserving -1 values for peaks where event numbers are not applicable.

Can you give a minimal working example (or illustrate with a figure)?

An example could be found at the wiki note : https://xe1t-wiki.lngs.infn.it/doku.php?id=xenon:xenonnt:analysis:analysts_overview_page:roi_frankel:Event_number_bug

Test with detailed example for bug and solution could be found at this notebook: https://github.com/XENONnT/analysiscode/blob/master/peak_per_event_corrected/new_peak_per_event_plugin.ipynb

coveralls commented 2 months ago

Coverage Status

coverage: 91.158% (+0.03%) from 91.125% when pulling 1dadb67b35d124c795479ffcfba65af0981dafaf on peak_per_event_bug_fix into 189c09e7c2c3c611d91b7040d462be1759f59f9e on master.

HenningSE commented 2 months ago

Hi @RoiFrankel thanks for the PR. Right now you have a long explanation of the proposed changes as comments in the code. It is of course nice to explain changes, but I would propose to move these comments from the code to the description of the PR otherwise the code will get difficult to track as more and more comments would pile up at some point.

RoiFrankel commented 2 months ago

Hi @HenningSE I tried to fix it according to your suggestions, I hope this is now fine.

RoiFrankel commented 2 months ago

Hi @yuema137 thanks for your respond ! I will add the unit test for the plugin :)

yuema137 commented 1 month ago

@RoiFrankel How's it going there? Don't worry if you don't have time to add the test. We can merge this PR without it.

yuema137 commented 1 month ago

@dachengx we can go ahead to merge this PR. I talked to Roi offline and we decided to add the test in the future.