-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add pulse merging functionality to GraphDefinition
#748
base: main
Are you sure you want to change the base?
Add pulse merging functionality to GraphDefinition
#748
Conversation
Just a minor comment/warning in |
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.
small typo but otherwise LGTM.
isinstance(value, (int, float)) | ||
for value in values_to_merge | ||
): | ||
# alculate the mean for all attributes except charge |
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.
small typo
This PR adds new functionality to
GraphDefinition
that allows it to merge coincident pulses/photons on the same PMT within somemerge_window
if themerge_coincident
argument is set toTrue
(defaults toFalse
). The merging is charge-weighted if charge is available in the input data.To avoid having to pass arguments to the
GraphDefinition
, the names of the time and charge columns are created asDetector
properties, and this PR adds the property for all existingDetector
s.Here's an example:
The corresponding output is
As you can see, the two first rows and the last two rows in the original data is same-pmt pulses (first is less than 2 ns apart and second is more than 6 ns apart). Because we chose a merging window of 4.5ns, only the first same-pmt pulses are merged.