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

Nveto changes #384

Merged
merged 24 commits into from Jan 22, 2021
Merged

Nveto changes #384

merged 24 commits into from Jan 22, 2021

Conversation

WenzDaniel
Copy link
Collaborator

@WenzDaniel WenzDaniel commented Jan 19, 2021

What is the problem / what does the code in this PR do
In this PR we make some optimizations to the hitlet data_type class.

  • We roughly halve the amount of Bytes stored per hitlet:
  • We reduced the area width and area decile fields to a 50% and 80% width and 10% and 80% decile. This turns out to be sufficient and allows to define rise and fall time
  • We added a new type of width which is estimated based on the highest density region of a distribution/signal. Since there was not any suited function I added one in prcoessing/statistics. Also here we keep always the left edge and width (50 % and 80% coverage) to compute rise and fall time if needed. See also the corresponding wikinote

This PR goes together with: XENONnT/straxen#319

@JoranAngevaare
Copy link
Member

What is the problem / what does the code in this PR do
In this PR we make some optimizations to the hitlet data_type class.
* We added a new type of width which is estimated based on the highest density region of a distribution/signal. Since there was not any suited function I added one in prcoessing/statistics. Also here we keep always the left edge and width (50 % and 80% coverage) to compute rise and fall time if needed. See also the corresponding wikinote

Hi Daniel, thanks for the note and the explanation. Like you also mention, it's nice to have somewhat consistent usage among NV & TPC, the downside for noisy wfs is granted but wouldn't you want to cut those anyway. I'm asking because I'd hate if in the end no one understand why there are two different widths in the hitlets.

Sorry for also asking it, but we should also really add tests for this. It's super easy to make a little mistake in abstract code.

@WenzDaniel
Copy link
Collaborator Author

Why should not we allow different parameters for a different type of signal/detector? I do not really understand this argument. In the end we have to see which parameter gives the best results/discrimination power and based on what I have seen so far the area deciles did not perform so well on our SPE signals.

Sorry for also asking it, but we should also really add tests for this. It's super easy to make a little mistake in abstract code.

Valid point, but I simply had not any time yet to add those.

Copy link
Member

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

Hi Daniel, thanks, it's very tarse material to review. I would argue it cannot do without tests but at this stage I'm not finding any obvious mistakes.

strax/processing/statistics.py Outdated Show resolved Hide resolved
@JoranAngevaare
Copy link
Member

JoranAngevaare commented Jan 20, 2021

Why should not we allow different parameters for a different type of signal/detector? I do not really understand this argument.

Sorry, this was not what I was trying to say. Was just wondering about the noisiness you describe in your note etc. I think it's really nice if we can have multiple algorithms in strax!

@WenzDaniel
Copy link
Collaborator Author

Hej, as requested I added more tests to the function which indeed also captured some nasty bugs. Further, I extended the documentation of the algorithm in https://xe1t-wiki.lngs.infn.it/doku.php?id=xenon:xenonnt:wenz:comissioning:nveto:hitlet_width#explanation_of_the_algorithm as well as in the code itself.

Copy link
Member

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

Thanks Daniel!

@WenzDaniel WenzDaniel merged commit b87402f into AxFoundation:master Jan 22, 2021
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.

None yet

2 participants