-
Notifications
You must be signed in to change notification settings - Fork 95
Calorimeter digi mixing #1678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Calorimeter digi mixing #1678
Conversation
|
Hi @edcallaghan,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for b3cee8b: build (Build queue - API unavailable) |
|
☔ The build is failing at b3cee8b.
N.B. These results were obtained from a build of this Pull Request at b3cee8b after being merged into the base branch at 5d5efb9. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for ae1489f: build (Build queue - API unavailable) |
|
☔ The build is failing at ae1489f.
N.B. These results were obtained from a build of this Pull Request at ae1489f after being merged into the base branch at 5d5efb9. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for fb026e0: build (Build queue - API unavailable) |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 479180c: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 479180c.
N.B. These results were obtained from a build of this Pull Request at 479180c after being merged into the base branch at 5d5efb9. For more information, please check the job page here. |
|
@bechenard I've requested your review based on authorship of existing code, but obviously please delegate/defer as appropriate. The real calorimeter action is in Also note that f56bda5 patches what I think is a (very minor) bug in the digitization which allows the simulation to produce adc readings which are one count above the true max reading, but I'll reverse that if I misunderstood. |
|
In the CI report, check_cmake is reporting a problem, please take a look at that. Thanks |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 71a8ca4: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 71a8ca4.
N.B. These results were obtained from a build of this Pull Request at 71a8ca4 after being merged into the base branch at 5d5efb9. For more information, please check the job page here. |
|
@rlcee thanks, I forgot to propagate new classes into |
This PR implements digi mixing for the calorimeter. The mechanics largely follow the pattern set by the tracker: the relevant data products are mixed using standard technology, and digis from the same channel which overlap in time are resolved into a single digi. For calorimeter digis, which are principally waveforms, this is conceptually straightforward: the waveforms are summed. It will, however, require extra effort to retain MC info for the calorimeter: unlike the tracker or CRV, which instantiate all MC info at the time of digi creation, it seems that some MC info for the calorimeter is reconstructed, making use of primary
SimParticleinfo, after digis have already been made and interpreted as hits. That should be fine, but it require extra book-keeping/massaging to work in situations where there is MC info (i.e. detector data) or some digis were digitized independently of the simulated signal (e.g. digi mixing with conventional pileup).New classes:
CaloDigiWrapper: A thin wrapper for calorimeter digis which is meant to collapse multiple loops, i.e. over synchronized digi + mc info collections, into a single loop. MC info is yet not included, for reasons described above.CaloDigiWrapperCollection: stl-vector of the above, with additional logic for resolving a set of overlapping digis into one.Changes to existing functionality:
Mu2eProductMixer: Include provisions for mixingCaloDigis andCaloShowerSims, which (it seems to me) is the principal data product, not already mixable, which will allow to retain MC info.TimeBasedBuckets: Generalized from a sequence of fixed-length timing windows to a sequence of variable-length timing windows. This is necessary as waveforms from the calorimeter of of variable length.Changes to existing modules:
MergeDigis: Resolve collisions between overlapping calorimeter digis usingCaloDigiWrapperCollectionfunctionality.Thoughts looking forward: The simple sum of multiple waveforms into a single waveform admits two subtleties: the saturation of the ADC must be reevaluated, and any analog noise present in the waveform is now "counted" multiple times in the sum.
The former is accounted for in this PR, albeit with the ADC bit depth configuration now duplicate, as it is supplied directly to the
CaloDigiMakermodule, and not factored into some kind of conditions class. It would be good for that to be done, or maybe move the definition to fcl-prolog, to avoid the possibility of them getting out of sync.The latter is not accounted for, as currently an implemented waveform is just that: a waveform; it does not know "what" it contains. Conceptually, one can imagine ways of keeping the noise contribution to waveforms distinct until it absolutely needs to be added to any signal present, but that is outside of the scope of this PR and I'm not in a position to recommend something right now. But it's something we'll want to get right, since potential applications include mixing digis from the detector with both detector data and MC, both in situations with overlapping digis and not, so artefacts associated with different levels of noise are possible.