CRV pulse hit index to replace pulses vector#376
Conversation
Guard MC fills when FillMCInfo is false, skip null CRV pulses and SimParticle chains, bound RooUtil index access for the validation macro, and repair the ceSimRecoVal FHiCL chain used by test_fcls. Co-authored-by: Cursor <cursoragent@cursor.com>
AndrewEdmonds11
left a comment
AndrewEdmonds11
left a comment
There was a problem hiding this comment.
Thanks for the PR, Rob! I have a few in-line comments below. There are a couple of things where the underlying cause might be elsewhere in the code so we should work out whether these are real fixes or just sticking plasters
| physics.producers.KKDe.KKFitSettings.SaveHitCalibInfo : true | ||
| outputs.Output.fileName : "mcs.owner.val-ceSimRecoVal.dsconf.seq.art" | ||
| // Production/Validation/ceSimReco drops CaloDigis; append keep so it wins over drop | ||
| outputs.Output.outputCommands[50]: "keep mu2e::CaloDigis_CaloDigiMaker_*_*" |
There was a problem hiding this comment.
If the intent is to append, then I suggest doing:
outputs.Output.outputCommands : [ @sequence::outputs.Output.outputCommands, "keep mu2e::CaloDigis_CaloDigiMaker_*_*" ]
Just in case the output commands in ceSimReco.fcl itself changes in future
| for (int i_crv_coinc = 0; i_crv_coinc < nCrvCoincs(); ++i_crv_coinc) { | ||
| CrvCoinc crv_coinc(&(crvcoincs->at(i_crv_coinc))); | ||
| if (crvcoincsmc != nullptr) { | ||
| if (crvcoincsmc != nullptr && i_crv_coinc < static_cast<int>(crvcoincsmc->size())) { |
There was a problem hiding this comment.
Are crvcoincs and crvcoincsmc branches not lock-step arrays?
There was a problem hiding this comment.
In Offline, CrvCoincidenceClusters and CrvCoincidenceClustersMC are in lock-step (if MC exists). However, in EventNtuple/src/CrvInfoHelper.cc line 114, we have a check that only results in a cout statement if a mismatch is found. I think it should throw an exception there, and perhaps here as well.
ehrlich-uva
left a comment
ehrlich-uva
left a comment
There was a problem hiding this comment.
I only looked at the CRV part of this PR.
I think this is a better way of dealing with the CrvRecoPulses.
…of bounds. Add warnings about false assumption of calo hit/MC alignment. The root cause of the seg faults/index out of bounds was in Events.hh, Event::Update() accessed calohitsmc->at(calohit_Idx) where calohit_Idx was an index into the calohits branch. Because calohitsmc is filled from the compressRecoMCs while calohits is filled from caloclusters.hits_, they are *not* aligned by definition. This caused vector::_M_range_check in validation: calohitsmc->at(4) when calohitsmc.size() == 3. Added warnings in the documentation and comments and preserved the pre-existing behavior of mapping reco hits->MC hits as if they were aligned. The other guards were tested and shown to be unnecessary.
|
@AndrewEdmonds11 most of the guards were unnecessary. That was an overzealous LLM agent papering over the real problem, which is an incorrect assumption about the calohits and calohitsmc branches being aligned. I preserved the original behavior of matching each reco calo hit to the MC calo hit with the same index while preventing an out of bounds vector access, but the real problem is upstream where the association object between the compressRecoMCs and the CaloHitMaker hits is never produced or stored, so there's no handle in the art file for EventNtuple to properly match MC truth hits with reco hits. |
3 participants
Replace the std::vector in CrvHitInfoReco with a flat int crvHitIndex in each CrvPulseInfoReco struct. The std::vector was not straightforward to unpack in pyutils and resulted in redundant data (the same pulse data stored in two separate branches). The new int index allows the pulses collected into a hit to be associated to it for analysis. A new KeepUnclusteredPulses FHiCL parameter (default false) allows to filter out reco pulses that were not collected into a hit.
In validation, discovered a segmentation fault and out-of-bounds vector element accesses. Those have been guarded so that test_fcls.sh runs cleanly.