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

fixed delta peak timestamp problem #702

Merged
merged 4 commits into from
Oct 17, 2022
Merged

fixed delta peak timestamp problem #702

merged 4 commits into from
Oct 17, 2022

Conversation

FaroutYLq
Copy link
Collaborator

What is the problem / what does the code in this PR do
The (lone hits) delta peaks added to merged_s2s waveforms have wrong time stamp. For example, in the example merged_s2s waveforms below, the red shadow marks peaklets while the blue shadow marks where we expect the lone hits, given the time stamps directly from lone_hits datatype. You can see the lone hits show up in the wrong time.
image
image
image

This PR did a minimal fix to this issue.

Can you briefly describe how it works?

index = (p['time'] - lh_i['time'])//p['dt']

is the problem. Apparently, lone hits' time should show up only after merged_s2s start. This implementation means that index is always negative, leading to a reversed ordering thus mirrored relative timestamp.

Can you give a minimal working example (or illustrate with a figure)?
I think this fix is simple enough to skip this part...

@coveralls
Copy link

coveralls commented Oct 14, 2022

Coverage Status

Coverage remained the same at 91.736% when pulling b292d5a on FaroutYLq:fix_add_lone_hits into e8b03fc on AxFoundation:master.

@JoranAngevaare
Copy link
Contributor

Great catch, must have been introduced in #506!

Thanks a lot Lanqing!

One day we may try again to not merge lone hits in te first place, especially if they are after pulses.

@WenzDaniel
Copy link
Collaborator

Thanks for catching Lanqing.

Rearding

One day we may try again to not merge lone hits in te first place, especially if they are after pulses.

One has to be a bit careful to not introduce in that case a z- and area dependent bias. This was one of the issues in the 1T Ar37 analysis as far as I remember.

@WenzDaniel
Copy link
Collaborator

Can you update the test such that we do not run into this issue ever again

def test_add_lone_hits(hits, peak_length):
?

peak['length'] = peak_length
peak['dt'] = 1
hits['area'] = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@WenzDaniel if we had realized this - the test would have worked. Now it always added zeros.

@JoranAngevaare JoranAngevaare merged commit 4fb2c08 into AxFoundation:master Oct 17, 2022
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.

4 participants