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

fix peak per event plugin #1400

Merged
merged 11 commits into from
Aug 25, 2024
Merged

fix peak per event plugin #1400

merged 11 commits into from
Aug 25, 2024

Conversation

RoiFrankel
Copy link
Contributor

@RoiFrankel RoiFrankel commented Jul 10, 2024

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

@RoiFrankel RoiFrankel marked this pull request as draft July 10, 2024 12:45
@RoiFrankel RoiFrankel requested a review from DonNabla July 10, 2024 13:04
@RoiFrankel RoiFrankel marked this pull request as ready for review July 10, 2024 13:05
@coveralls
Copy link

coveralls commented Jul 10, 2024

Coverage Status

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

@HenningSE
Copy link
Contributor

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 and others added 2 commits July 10, 2024 17:02
Comments were moved from code to description of PR
@RoiFrankel
Copy link
Contributor Author

RoiFrankel commented Jul 10, 2024

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

@RoiFrankel RoiFrankel closed this Jul 10, 2024
@RoiFrankel RoiFrankel reopened this Jul 10, 2024
@yuema137 yuema137 assigned yuema137 and unassigned yuema137 Jul 10, 2024
@yuema137 yuema137 self-requested a review July 10, 2024 18:42
Copy link
Collaborator

@yuema137 yuema137 left a comment

Choose a reason for hiding this comment

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

Hi @RoiFrankel good catch and nice implementation! I went through your note and module in analysis code, which are quite clear. As you already have those checks in hand, can I ask you for another favor to add an unit test for this plugin, i.e. create a peak_per_event.py under straxen/tests/plugins?
Don't worry if you don't want to do it as it's not necessary.
Thanks a lot :)

The commit by me is just to remove the redundant # in the comment to resolve the error from pre-commit checking. Nothing changed functionally.

@yuema137 yuema137 self-requested a review July 10, 2024 18:58
yuema137
yuema137 previously approved these changes Jul 10, 2024
@RoiFrankel
Copy link
Contributor Author

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

@yuema137
Copy link
Collaborator

yuema137 commented Aug 5, 2024

@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
Copy link
Collaborator

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

@dachengx dachengx merged commit 1dd8e3e into master Aug 25, 2024
8 checks passed
@dachengx dachengx deleted the peak_per_event_bug_fix branch August 25, 2024 12:39
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.

5 participants