Fixing bug with NN in strip pairing - #218
julianmgerber wants to merge 44 commits into
Conversation
CHG: Speed optimizations
Cherry-picked this commit from PR148 to apply seperately
…arestneighborstriphit" function to MHit class
ckierans
left a comment
ckierans
left a comment
There was a problem hiding this comment.
Looks good, Julain! I'm good to merge. But please see my comment about the definition of the IncludingNearestNeighbors boolean and a longer-term fix for that.
| MStripHit* SH = Event->GetStripHit(sh); | ||
|
|
||
| if (SH->IsNearestNeighbor() == true) { | ||
| if ((SH->IsNearestNeighbor() == true) && (IncludingNearestNeighbors == false)) { // if seeing a NN strip for the first time, set to true |
There was a problem hiding this comment.
This self-definition of "IncludingNearestNeighbors" is giving me pause again. What if there is somewhat garbage data and a NN striphit gets through, but you don't actually want to analyze NN striphits? This would trip you, right?
I get the point about not having the HDF data loader as a prerequisite for this module, so you might not have access to the Get/SetIncludeNearestNeighbors already defined in the MModuleLeaderMeasurementHDF. But what we probably should do is have a Get/SetIncludeNearestNeighbors in every data loaded/reader. That's out of scope with what're your trying to do here, of course. But, if you agree, then let's make an issue and revisit this IncludingNearestNeighbors at a later time.
There was a problem hiding this comment.
Yeah, that definitely makes sense!
There was a problem hiding this comment.
Should there also be an issue for the NNs without energy calibrations?
There was a problem hiding this comment.
Alternatively: would it make sense to add some m_IncludeNearestNeighbors to ReadOutAssembly, such that Event already contains that information?
Then whatever loader module created the Event can set that.
|
Why are there so many commits in this PR? 😅 |
@fhagemann I'm not sure why it's showing as so many commits... All of the commits except the last 4 are already in the develop/em branch. |
|
Ah, your PR #183 got squashed, so all the commits in there were combined into one. |
4 participants
When comparing the outputs from the strip pairing with and without the nearest neighbors, I found that there are an additional ~30,000 events that pass through event selections when including NN. It turns out these are events that have no triggered strips but do have NN strips. Since there are NN strips that are output by the "Event->GetNStripHits" command, they don't get the "No strip hits" BD flag.
I edited the strip pairing so that only the triggered strips are actually counted, and events with no triggered strips are flagged as BD. I also updated the unit test references to reflect the changed BD flag.
A separate (but related) issue is that, when including NNs, ~20% of events are lost at the energy calibration stage because the NN happens to fall on a strip that doesn't have a valid energy calibration. The triggered strips are still able to be calibrated, though. I haven't edited the energy cal module because I wanted to check first, but I assume we don't want to be discarding these events just based on the NNs?