-
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 |
henrylay97
left a comment
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.
Hi Jacob, thanks for the PR. I have a few comments.
My main concern is around the fix for the empty event problem. Rather than finding the root cause of the problem, the solution seems to be to provide a second vector that you hope won't be empty as well.
I would rather the actual cause of the segfault be found and prevented. This would also remove any need for hard coded module labels.
The rest looks good to me, happy to discuss more offline!
| if(evt.getByLabel("specialblipgaushit",hitHandleGH)) art::fill_ptr_vector(hitlistGH, hitHandleGH); | ||
| else if(evt.getByLabel("gaushit",hitHandleGH)) art::fill_ptr_vector(hitlistGH, hitHandleGH); |
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.
Module labels should never be hard coded. I appreciate this was true before this PR but given you're touching the code at this point it needs fixing.
I also don't understand the use of two different hit collections - only one of them will have been an input to the BlipReco and so only one of them is a valid input for the truth matching. If the code crashes on an empty vector somewhere then the fix should be a catch to stop that happening rather than the providing of a different vector.
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.
I have removed the hitHandleGH, and will remove the other spots that gaushit gets a special default privileging. It will just be the user's responsibility to make sure they have ran all the necessary inputs
… some branches to assume the hitHandle the user provided was run through truthmatching and pandoraTrack
|
Thanks for the suggestions, most are in. Let me check compilation and a simple output in a typical Fall production run |
|
Merged in develop and the compiler works and entries in the reco2 files look good. After getting the normal successful processing table I got this error |
|
Fixed the above error. |
|
Thanks so much for responding so quickly. Looking much better. I have a question. Will this now not segfault on empty events? Regardless of the label being applied. I note you say: "I have removed the hitHandleGH, and will remove the other spots that gaushit gets a special default privileging. It will just be the user's responsibility to make sure they have ran all the necessary inputs" Does this mean if the user mislabels the input we get a segfault? If so we need to avoid that and instead have a more graceful failure mode :) |
|
I'll check that now. Noting down the possible combinations of wrong label:
|
|
We don't ever segfault. But we do have some cases where a normal output will be produced and the user must be careful that the fcl used to make it was correct. For case 1 For case 2 For case 3: For case 4: For case 5: |
henrylay97
left a comment
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.
Brilliant. Thanks for the detailed checks Jacob! There is always some need for analysers / devs / release management to be careful with fcl labels so the responses to 2 and 3 are reasonable. Overall I'm happy that this will fail gracefully if at all.
As long as you're happy with the production fcl state then I'm happy with this now :D
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.