Overlay workflow fixes [develop] [2/4] - #671
Open
PetrilloAtWork wants to merge 14 commits into
Open
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
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).
Removed some unused or redundant code.
* added `CAFMaker::GetHandleStrict()` that directly returns a handle * `GetByLabelStrict()` and similar now use `art::InputTag` as label type * minor implementation changes and documentation fixes
* removed the support for "unshifted trigger" as a reference
* removed any support for different shifts in data and simulation
* the only supported shift is from trigger time to beam gate time
* both are learnt from `DetectorClocksService`
* it is assumed that the current data is on trigger reference; we don't have a general way to check that
* it is _possible_ to extend the system so that, given as parameter the value of the current reference (`1500`), then we can move to trigger time, beam time or any other time as reference, and also verify if we are already there. For once, I haven't gone all the way toward fancy and this is not implemented yet.
* an additional global shift is also supported
* an additional shift for CRT only is also supported
* time reference shifting moved from central place (one place, all shifts) to object filling places (many places, one object each); the latter was already the case for ICARUS CRT (at least some of it)
* filler functions are passed "shifter objects" which know which shift to apply
* time shift applied to:
* trigger information (shared; SBND should inherit the shift as before): `beam_gate_det_time`, `global_trigger_det_time`
* CRT hit collection: `t0`, `t1`, `time`
* CRT track collection: `time` (not sure if it is correct when using T0)
* CRT/PMT match collection: `flashTime_us`, `firstOpHitPeakTime`, `firstOpHitStartTime`, `matchedCRTHits.time`
* optical flashes
* ICARUS: `time`, `firstTime`
* SBND: `time`, `firstTime`
* slice TPC/PMT matching (OpT0Finder): `opt0.time`, `opt0_sec.time`
* slice TPC/PMT matching (barycentre): `barycenterFM.flashFirstHit`, `barycenterFM.flashTime`
* slice TPC/PMT matching (simple): `time`
* CRT hits for a track: same as for the event-wide collection
* CRT track for a track: `crttrack.time`
* removed configuration parameters (**breaking change**)
* `UnshiftedTriggerLabel`
* `CRTSimT0Offset`
* `ReferencePMTFromTriggerToBeam`
* `ReferenceCRTT0ToBeam`
* `ReferenceCRTT1FromTriggerToBeam`
* added configuration parameters:
* `ShiftTimeFromTriggerToBeamGate`
* `GlobalTimeReferenceOffset`
* `CRTreferenceTimeOffset`
PetrilloAtWork
changed the title
develop] [2/4]
Contributor
|
Putting this down here; forwarding to SBND. Thanks, @PetrilloAtWork ! |
Contributor
|
I agree with @PetrilloAtWork that this would break SBND Data. For these changes in CAFMakers, they affect some data products shared between SBND & ICARUS: OpFlash/OpT0/BFM. |
| slice.correctedOpFlash.setDefault(); | ||
| if ( slcCorrectedOpFlash.empty()==false ) { | ||
| const sbn::CorrectedOpFlashTiming &_correctedOpFlash = *slcCorrectedOpFlash[0]; | ||
| slice.correctedOpFlash.OpFlashT0 = _correctedOpFlash.OpFlashT0; |
Contributor
There was a problem hiding this comment.
Suggested change
| slice.correctedOpFlash.OpFlashT0 = shifter.shiftedTime(_correctedOpFlash.OpFlashT0); |
Hi! I wasn’t assigned as a reviewer, but I had a quick look at the PR. Just for completeness, although ICARUS is not currently using correctedOpFlash objects, they also include timing information that should be shifted accordingly
| slice.correctedOpFlash.OpFlashT0 = _correctedOpFlash.OpFlashT0; | ||
| slice.correctedOpFlash.NuToFLight = _correctedOpFlash.NuToFLight; | ||
| slice.correctedOpFlash.NuToFCharge = _correctedOpFlash.NuToFCharge; | ||
| slice.correctedOpFlash.OpFlashT0Corrected = _correctedOpFlash.OpFlashT0Corrected; |
Contributor
There was a problem hiding this comment.
Suggested change
| slice.correctedOpFlash.OpFlashT0Corrected = shifter.shiftedTime(_correctedOpFlash.OpFlashT0Corrected); |
See #671 (comment)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is part of the fixes to the overlay workflow delivered for production branch by #653, recast to
develop. The description of the content can be found in that request.The top pull request for
developis SBNSoftware/icaruscode#922.While this content was already approved and merged in the production branch, I would like to make sure that it is met with the appropriate awareness.
This PR affects the content of CAF and I have not tested it on any SBND data.
A feature, time shifting, has been modified/removed: I think SBND was not using it, but I am not sure. Also, this change was extensive, touching all the time variables I could spot; each change is a chance to introduce a bug. I have not seen anything wrong with ICARUS data, but SBND data mileage may vary.
I am available to help with testing, if provided appropriate instructions.
Reviewers: the SBN release managers to increase awareness.
This PR is required by SBNSoftware/icaruscode#922 and I think does not depend on other PR.