From 90abf94b641f838695a337774110d3aea7f47726 Mon Sep 17 00:00:00 2001 From: Yuri Oksuzian Date: Sat, 29 Aug 2026 14:30:24 -0500 Subject: [PATCH] CRVResponse: fix defects found by a static sweep of the package CrvPhotonGenerator dedups MakeCrvPhotons instances by lookup-table filename and shares one instance between sectors, but SetScintillationYield is only called on the first-load branch, so the shared instance keeps the first sector's yield. In prolog_v12.fcl, T3/T4/T5 all name LookupTable_6000_0 with yields 39794, 39794, 30482: T3 loads first, T4 matches by luck, and T5 silently runs at 39794 instead of its configured 30482, a +30.5% light-yield overestimate. It is the only mismatched filename group in the file, and the debug print reports the configured value rather than the one in use, so the log conceals it. The same filenames and yields are in prolog_v11.fcl; v10 and v09 list only T1-T4, all at 39400, so this entered when T5 was added at v11. Dedup now requires the yields to agree as well as the filename. MakeCrvSiPMCharges::FindFiberPhotonsPixelId returned lrint of an unbounded GetRandom2 sample, so the outermost bin could yield pixel id 40 in a 40x40 array. That phantom pixel carried its own discharge state. Clamp to the array. MakeCrvPhotons::LoadLookupTable never checked the stream after reading the Cerenkov map and bin definitions, so a truncated table left bin boundaries and yield curves holding whatever was in the buffers and the job continued with silently wrong photon yields. Throw instead. MakeCrvDigis::SetWaveform narrowed each sample to int16_t before applying the [minADC,maxADC] clamp, so an extreme sample wrapped around and landed back inside the range instead of saturating. Clamp in double, then narrow. MakeCrvWaveforms::LoadSinglePEWaveform called max_element on the waveform vector without checking it is non-empty. CrvWidebandTest reset the five track-fit arrays after the continue that skips trigger sector types, so for every index in triggerSectorTypes those branches held whatever new[] returned on event 1 and were persisted to the ntuple every event. Reset before the continue. Its destructor also freed 3 of its 18 new[] arrays. CrvPlot dereferenced four art::Handles with no isValid() check, where CrvMCHelper and CrvCoincidenceClusterMatchMC guard the same case; declared `double scale[4]={NAN}` and `maxADC[4]={NAN}`, which sets only element 0 to NAN and value-initializes 1-3 to 0.0, defeating the isnan() "no digis" guard for SiPMs 1-3; and its CMakeLists omitted Offline::GlobalConstantsService despite calling GlobalConstantsHandle. Found by a static review of the package at 0ef02a66d. Not built locally -- relying on CI for the build. Co-Authored-By: Claude Opus 5 --- CRVResponse/CMakeLists.txt | 1 + CRVResponse/src/CrvPhotonGenerator_module.cc | 5 +++- CRVResponse/src/CrvPlot_module.cc | 19 +++++++++++-- CRVResponse/src/CrvWidebandTest_module.cc | 29 ++++++++++++++++---- CRVResponse/src/MakeCrvDigis.cc | 10 ++++--- CRVResponse/src/MakeCrvPhotons.cc | 4 +++ CRVResponse/src/MakeCrvSiPMCharges.cc | 6 +++- CRVResponse/src/MakeCrvWaveforms.cc | 2 ++ 8 files changed, 63 insertions(+), 13 deletions(-) diff --git a/CRVResponse/CMakeLists.txt b/CRVResponse/CMakeLists.txt index a7ce7b1c07..e2b669a3ff 100644 --- a/CRVResponse/CMakeLists.txt +++ b/CRVResponse/CMakeLists.txt @@ -66,6 +66,7 @@ cet_build_plugin(CrvPlot art::module Offline::CRVConditions Offline::DataProducts Offline::GeometryService + Offline::GlobalConstantsService Offline::MCDataProducts Offline::ProditionsService Offline::RecoDataProducts diff --git a/CRVResponse/src/CrvPhotonGenerator_module.cc b/CRVResponse/src/CrvPhotonGenerator_module.cc index fee11ffdf0..7eb0529f0e 100644 --- a/CRVResponse/src/CrvPhotonGenerator_module.cc +++ b/CRVResponse/src/CrvPhotonGenerator_module.cc @@ -198,7 +198,10 @@ namespace mu2e bool tableLoaded=false; for(size_t j=0; j crvRecoPulseCollection; event.getByLabel(_crvRecoPulsesModuleLabel,"",crvRecoPulseCollection); + // any of these products can legitimately be absent from a file whose intermediate CRV + // collections were dropped, so say which one is missing instead of dereferencing an + // invalid handle (CrvMCHelper.cc and CrvCoincidenceClusterMatchMC_module.cc guard the same way) + if(!crvPhotonsCollection.isValid()) + throw cet::exception("SIM")<<"mu2e::CrvPlot: no CrvPhotonsCollection with label "<<_crvPhotonsModuleLabel< CRS; @@ -233,8 +246,10 @@ namespace mu2e //waveforms std::vector > ADCs[4]; //there can be multiple disconnected ADC waveforms per SiPM std::vector > times[4]; - double scale[4]={NAN}; - double maxADC[4]={NAN}; + // NB: `double x[4]={NAN};` would set only element 0 to NAN and value-initialize 1-3 to 0.0, + // which silently defeats the isnan() "no digis" guard below for SiPMs 1-3 + double scale[4]={NAN,NAN,NAN,NAN}; + double maxADC[4]={NAN,NAN,NAN,NAN}; CrvDigiCollection::const_iterator digis; for(digis=crvDigiCollection->begin(); digis!=crvDigiCollection->end(); ++digis) { diff --git a/CRVResponse/src/CrvWidebandTest_module.cc b/CRVResponse/src/CrvWidebandTest_module.cc index 171ce092cb..87a5532823 100644 --- a/CRVResponse/src/CrvWidebandTest_module.cc +++ b/CRVResponse/src/CrvWidebandTest_module.cc @@ -241,8 +241,23 @@ namespace mu2e CrvWidebandTest::~CrvWidebandTest() { delete[] _recoPEs; + delete[] _recoTime; + delete[] _fitStatus; delete[] _depositedEnergy; delete[] _coincidencePDGid; + delete[] _coincidenceTime; + delete[] _coincidencePosX; + delete[] _coincidencePosY; + delete[] _coincidencePosZ; + delete[] _trackSlope; + delete[] _trackIntercept; + delete[] _trackPoints; + delete[] _trackPEs; + delete[] _trackChi2; + delete[] _summaryPEs; + delete[] _summaryFWHMs; + delete[] _summarySignals; + delete[] _summaryChi2s; } void CrvWidebandTest::beginJob() @@ -396,6 +411,15 @@ namespace mu2e //fits for the entire stack of modules/sectors (sectorType=0) and for individual modules/sectors (sectorType=1,...) for(int iSectorType=0; iSectorType<_nSectorTypes; ++iSectorType) { + //reset the per-event track variables for EVERY sector type, including the trigger sectors + //skipped below: these arrays are branched into the tree at dimension _nSectorTypes and are + //filled every event, so an un-reset trigger-sector entry would persist whatever new[] left there + _trackSlope[iSectorType] =0; + _trackIntercept[iSectorType]=0; + _trackPEs[iSectorType] =0; + _trackPoints[iSectorType] =0; + _trackChi2[iSectorType] =-1; + if(std::find(_triggerSectorTypes.begin(),_triggerSectorTypes.end(), iSectorType) != _triggerSectorTypes.end()) continue; //initialize track variables @@ -403,11 +427,6 @@ namespace mu2e float sumY =0; float sumXY =0; float sumYY =0; - _trackSlope[iSectorType] =0; - _trackIntercept[iSectorType]=0; - _trackPEs[iSectorType] =0; - _trackPoints[iSectorType] =0; - _trackChi2[iSectorType] =-1; double counterWidth = counters.at(0)->getBarDetail().getHalfWidth()*2.0; int widthDirection = counters.at(0)->getBarDetail().getWidthDirection(); //assumes that all counters are oriented in the same way diff --git a/CRVResponse/src/MakeCrvDigis.cc b/CRVResponse/src/MakeCrvDigis.cc index daaf467dc4..567e7cb407 100644 --- a/CRVResponse/src/MakeCrvDigis.cc +++ b/CRVResponse/src/MakeCrvDigis.cc @@ -9,10 +9,12 @@ void MakeCrvDigis::SetWaveform(const std::vector &waveform, double ADCco _ADCs.resize(waveform.size()); for(size_t i=0; i(waveform[i]*ADCconversionFactor+pedestal+0.5); - if(ADCmaxADC) ADC=maxADC; - _ADCs.at(i)=ADC; + // clamp before narrowing: an extreme sample cast to int16_t first would wrap around and + // land back inside [minADC,maxADC], so the saturation clamp would never see it + double ADCd = waveform[i]*ADCconversionFactor+pedestal+0.5; + if(ADCdmaxADC) ADCd=maxADC; + _ADCs.at(i)=static_cast(ADCd); } int TDCtmp=lrint(startTime/digitizationPrecision); diff --git a/CRVResponse/src/MakeCrvPhotons.cc b/CRVResponse/src/MakeCrvPhotons.cc index cbf4e18027..bf1d7fcfd2 100644 --- a/CRVResponse/src/MakeCrvPhotons.cc +++ b/CRVResponse/src/MakeCrvPhotons.cc @@ -275,6 +275,10 @@ void LookupBin::Read(std::ifstream &lookupfile, const unsigned int &i) for(unsigned int i=0; i0) std::cout<<"Done."< MakeCrvSiPMCharges::FindFiberPhotonsPixelId() { double x,y; _photonMap->GetRandom2(x,y); - return std::pair(lrint(x),lrint(y)); + // GetRandom2 samples the full width of the outermost bin, so lrint can return _nPixelsX/Y, + // one past the last pixel; that phantom pixel would carry its own discharge state + int ix=std::min(std::max(static_cast(lrint(x)),0),_nPixelsX-1); + int iy=std::min(std::max(static_cast(lrint(y)),0),_nPixelsY-1); + return std::pair(ix,iy); } bool MakeCrvSiPMCharges::IsInactivePixelId(const std::pair &pixelId) diff --git a/CRVResponse/src/MakeCrvWaveforms.cc b/CRVResponse/src/MakeCrvWaveforms.cc index 1e05466b83..a5bbd82e26 100644 --- a/CRVResponse/src/MakeCrvWaveforms.cc +++ b/CRVResponse/src/MakeCrvWaveforms.cc @@ -48,6 +48,8 @@ void MakeCrvWaveforms::LoadSinglePEWaveform(const std::string &filename, double } f.close(); + if(_singlePEWaveform.empty()) + throw std::logic_error("Could not read any single PE waveform points from "+filename); _singlePEMaxVoltage = *std::max_element(_singlePEWaveform.begin(), _singlePEWaveform.end()); }