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

Dynamic led window #1401

Merged
merged 37 commits into from
Aug 25, 2024
Merged

Dynamic led window #1401

merged 37 commits into from
Aug 25, 2024

Conversation

tflehmke
Copy link
Contributor

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

This PR implements a dnyamic window for the LED pulse integration in the afterpulse_processing and led_calibration plugins.

Can you briefly describe how it works?

It uses the straxen hit finder to find the first hit inside a fixed, wider window in the record. In the afterpulse plugin, all hits around this hit inside the LED_window_width are then integrated to give the full LED hit. In led calibration plugin, the samples around the first hit given by led_hit_extension are integrated and the maximum is taken from this area.

Please include the following if applicable:

  • Update the docstring(s)
  • Tests to check the (new) code is working as desired.

@tflehmke tflehmke requested a review from yuema137 July 30, 2024 12:16
@GiovanniVolta GiovanniVolta self-requested a review July 30, 2024 12:17
@GiovanniVolta GiovanniVolta requested a review from hoetzsch July 30, 2024 12:18
@coveralls
Copy link

coveralls commented Jul 30, 2024

Coverage Status

coverage: 90.865% (-0.3%) from 91.138%
when pulling 0e2cd92 on dynamic_led_window
into 1dd8e3e on master.

@dachengx dachengx self-requested a review August 6, 2024 14:36
@yuema137
Copy link
Collaborator

yuema137 commented Aug 6, 2024

Hi @tflehmke Many thanks to this very low-level PR! I went through the two updated files and think they are mostly good. The only possible functional problem that I found is that:

  • In led_calibration.py, hits are calculated by the find_hits function in strax, which are not sorted yet (in the afterpulse plugin they are sorted). If we want to only keep the first hit by time, we need to add the sorting here.

I also have some other minor suggestions for naming/comments/changing hard-coded numbers to URLConfigs, please see the single comments for details.

For the speed and memory I'm not really worried, as the calculation of dynamic windows is still O(N), and the size of the dynamic window is much smaller than the hits themselves. But it will still be great if you could provide me an example dataset so that I can test the performance.

@@ -33,7 +37,7 @@ class LEDCalibration(strax.Plugin):
from the signal one.
"""

__version__ = "0.2.4"
__version__ = "0.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to go to 1.0.0

hit. :return (len(records), 2) array: Integration window for each record

"""
hits = strax.find_hits(
Copy link
Contributor

@GiovanniVolta GiovanniVolta Aug 7, 2024

Choose a reason for hiding this comment

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

What is the lowest hit detectable? 15 ADC? Can this bias the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know how to test the plugin in place in straxen earlier, so I copied the function and run it outside. However doing that I did not have access to the URL config resources. I tested it again in place and it turns out with the hard cutoff of 15 ADC I see a hard cut in the amplitude spectra at 15 ADC. I removed the hard cutoff and always use the hit_min_height_over_noise which removes the hard cut in the spectra.

@sebvetter sebvetter self-requested a review August 16, 2024 07:15
@yuema137 yuema137 self-requested a review August 19, 2024 15:47
yuema137
yuema137 previously approved these changes Aug 19, 2024
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.

It looks all good to me now. Thank you @tflehmke !

Comment on lines +135 to +137
LED_hit_left_boundary,
LED_hit_right_boundary,
LED_window_width,
Copy link
Collaborator

@dachengx dachengx Aug 25, 2024

Choose a reason for hiding this comment

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

These arguments should be in principle in the snake case. But it is fine to keep things unchanged here to be compatible with the name of configs.

@dachengx dachengx merged commit ced9f51 into master Aug 25, 2024
8 checks passed
@dachengx dachengx deleted the dynamic_led_window branch August 25, 2024 18:57
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