Feature/gp overlay fix#653
Conversation
Contains `sbn::RebindAssociatedProducts()` to make a copy of an association with the data product on one side replaced by another one.
…rms and baselines
mvicenzi
added
enhancement
Prevents things like magic "invalid" values like -9999.0 from becoming -9997.8 after a 1.2 us shift. Introduced an utility `caf::SRdefaults` for fetching those magic values.
Must be explicitly disabled if not desired. The data is currently not shifted (not clear what a shift on timestamps would mean).
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
Tested with ICARUS Run2 overlay simulation, both from scratch and reprocessing.
Works ok.
There was a problem hiding this comment.
Pull request overview
This PR extends time-shifting support around trigger overlays by enhancing AdjustSimForTrigger outputs (shifted raw::Trigger, optional sbn::ExtraTriggerInfo, and rebinding of waveform–baseline associations) and by adding small utilities to preserve semantic defaults in CAF-making.
Changes:
- Add
sbncode/Utilitieswith a header-only association rebinding helper (sbn::RebindAssociatedProducts). - Update
AdjustSimForTriggerto (optionally) emit shifted trigger products, duplicatesbn::ExtraTriggerInfo, and rebindraw::OpDetWaveform–icarus::WaveformBaselineassns when shifting waveforms. - Add
caf::SRDefaultsand use it inCAFMakerto avoid time-shifting Standard Record fields that are still at their “defaulted” value.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| sbncode/CMakeLists.txt | Adds the new Utilities subdirectory to the build. |
| sbncode/Utilities/CMakeLists.txt | Defines a header-only Utilities interface library target and installs headers/sources. |
| sbncode/Utilities/AssnsUtils.h | Introduces sbn::RebindAssociatedProducts() for one-to-one association pointer rebinding. |
| sbncode/DetSim/CMakeLists.txt | Updates DetSim module link libraries and adds dependency on sbncode::Utilities. |
| sbncode/DetSim/AdjustSimForTrigger_module.cc | Expands functionality: shifted trigger output, optional ExtraTriggerInfo copy, optional waveform–baseline assn rebinding, and updated documentation. |
| sbncode/CAFMaker/SRDefaults.h | Declares caf::SRDefaults for retrieving “defaulted” Standard Record object values. |
| sbncode/CAFMaker/SRDefaults.cxx | Implements static initialization of defaulted Standard Record objects via setDefault(). |
| sbncode/CAFMaker/RecoUtils/CMakeLists.txt | Adds sbnanaobj::StandardRecord dependency. |
| sbncode/CAFMaker/CAFMaker_module.cc | Uses SRDefaults to avoid shifting invalid/default flash-match timing fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| art::InputTag fBindWaveformBaselines; ///< Tag of OpDetWaveform-baseline associations to be rebound. | ||
| double fAdditionalOffset; | ||
| bool fDropTriggerProduct; ///< Do not put the shifted trigger data product into the event. | ||
| bool fSkipExtraTriggerInfo; ///< Copy input `sbn::ExtraTriggerInfo`. |
|
Conflicts will be resolved at the time when the merge destination branch is finalised. |
francescopoppi
left a comment
francescopoppi
left a comment
There was a problem hiding this comment.
This one was a bit easier to track. Approved.
I believe there is rooms to make improvement for the future for additional variables to be filled in the CAFs (there are variabile which are unused by ICARUS so far, but filled by SBND, I think with these PR in place, we could also exploit them for additional handles in the selection)
Description
This PR supports improvements to the trigger shifting module for overlays, included nut not limited to:
AdjustSimForTriggerproduces araw::Triggerdata product for future time reference;AdjustSimForTriggerreproduces associations with PMT baselines;AdjustSimForTriggerproduces a "shifted"sbn::ExtraTriggerInfotoo;CAFMakerdoes not time-shift some time variables when their value is invalid.This is required by: