-
Notifications
You must be signed in to change notification settings - Fork 54
Bugfix/blips on empty events #901
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
base: develop
Are you sure you want to change the base?
Conversation
|
https://docs.google.com/presentation/d/1igXPcS_Qst99layiHuMEZiUxz4EHMxGHutf7Mr-jbT0/edit?slide=id.g3bbdbff95f2_0_1#slide=id.g3bbdbff95f2_0_1 here is a useful overview set of slides for these changes. Sorry for the many commits most are just adding/removing debug. For the hardcoded gaushit parameter the change in this PR is sufficient for any standard running someone does/for Fall production. Longterm I think these should not be hardcoded and just use the fcl-provided hit collection, and we assume the user knows what they are doing. The existing MC truth back tracking works. I just added a few additional breaks in MC truth blips. I found a way to get labeling I am totally happy with but the performance hit is too large for anything other than a small production aimed specifically at blips. I am happy with the current state of the MCTruth matching in this PR now. I promise some future slides looking at how the MC matching is doing in Fall production pre/post this PR in the next ~week |
A few bugs existed in old versions of blip reco
The first bug was related to a hardcoded hitHandleGH label, which looked at gaushit rather than specialBlipGausHit. On ~emtpy events the lower threshold (specialBlipGausHit) will have a few entries, but the code uses hitHandleGH for truth matching, and that higher threshold hit collection can be empty. That leads to crashes.
For now the hardcoded label is just updated to accommodate specialBlipGausHit.
The second bug is not critical, as access hitclusters this way is done through a blip, so the ID is the same as the blip you are looking at and the clusters included must be matched (into this blip). The hitcluster collection values are updated after the struct assignment (copy constructor), so I updated the blip alg to also update the blip.Cluster[i].blipID, and isMatched.
The third bug I am still digging into. I was worried that the switch from sim::energy_deposition to sim::channel->IDE was causing issues, but in the new arrangement (with an optional interface to use either) gives consistent results between them.
I have some notes I can build into slides for the first two but would like to spend a little more time on the third bug.