Fix FE chip stub truncation in TTStubBuilder #30648
Conversation
|
@tomalin, CMSSW_11_2_X branch is closed for direct updates. cms-bot is going to move this PR to master branch. |
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30648/16926
|
|
A new Pull Request was created by @tomalin (Ian Tomalin) for master. It involves the following packages: L1Trigger/TrackTrigger @benkrikler, @civanch, @kpedro88, @cmsbuild, @rekovic, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins.
|
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| bool lowerOK = false; | ||
| bool upperOK = false; | ||
|
|
||
| for (clusterIter = lowerClusters.begin(); clusterIter != lowerClusters.end() && !lowerOK; ++clusterIter) { |
There was a problem hiding this comment.
The call to edmNew::makeRefTo() (defined in DetSetNew.h) inside this loop demands an iterator to the cluster as its argument. The range-based loop provides this, whereas a C++11 type loop would not.
| } | ||
| } | ||
|
|
||
| for (clusterIter = upperClusters.begin(); clusterIter != upperClusters.end() && !upperOK; ++clusterIter) { |
There was a problem hiding this comment.
Identical response as previous comment to #30648 (comment)
There was a problem hiding this comment.
@tomalin , I just have signed the PR, however, thinking more I would say, that this loop is potentially dangerous, because "upperClusters.end() && !upperOK" may never happens. If I am wrong and this happens in all cases, why you check "!upperOK" at each iteration? If it is needed to interrupt the loop earlier it is possible to do "break" inside the loop.
|
+1 |
I just want to comment that in simulation even with one thread the adjacent events do not correspond to adjacent bunch crossings (assuming the "previous 8 events" really means "previous 8 bunch crossings"), so simulating such an effect would require a different approach (that, if I recall correctly, is being used by other subdetector(s)). |
|
@makortel If you do remember somewhere else in CMS, where the hit readout efficiency in one bunch crossing depends on the number of hits in the previous few bunch crossings, and this was simulated in a robust fashion, please let me know. We've never found a good way of doing this. Though any such improvement would be for a future PR, not for this one. |
|
I do not think that simhit/digi merging is affected by multi-threading. |
|
Pixels do this I believe. Anyway the previous few bunch crossings are available within the mixing module in digis (iirc).
On Jul 22, 2020, at 3:59 PM, Ian Tomalin <notifications@github.com<mailto:notifications@github.com>> wrote:
@makortel<https://github.com/makortel> If you do remember somewhere else in CMS, where the hit readout efficiency in one bunch crossing depends on the number of hits in the previous few bunch crossings, and this was simulated in a robust fashion, please let me know. We've never found a good way of doing this. Though any such improvement would be for a future PR, not for this one.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#30648 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ7QI7FRS2MAUIS6MDTR43WEDANCNFSM4OXM6G3A>.
|
My recollection points to pixels as well. Anyway the digitizers in MixingModule have access to the full pattern of SimHits from the simulated out-of-time pileup bunch crossings (that is, IIRC, -12..+3 for Runs 1-3 and -3...+3 for Run 4). |
|
Indeed, the correct approach is using the previous bunch crossings provided during pileup mixing. There are some examples of accessing bunch crossing information from the |
|
Hi All - Unfortunately, to make this work properly, you actually need to make digis and then run the code to generate the L1 Track Trigger primitives (stubs) for all of the preceding out-of-time bunch crossings, assuming that the sim hits in those crossings are actually "in-time" as far as the electronics are concerned. This breaks PreMixing, for example, since all of the sim hits would have to be saved in the PreMixed files. Just accessing the average occupancy in previous bunch crossings from the PileupSumaryInfo object doesn't provide enough information. We could do this in a stand-alone mode, which is the eventual plan. For production, I think the only way to make this work is to just have an average truncation probability based on expected occupancy, like the Pixel code. |
|
Does that imply that the primitives/stubs generation should be run during premix stage 1 and provide a collection of out-of-time stubs that should be consumed during premix stage 2? |
|
But then you don't have the overlap between pileup stubs and the hard scatter... |
|
Oh, wait, I see what you are suggesting - make all of the out-of-time stubs but do the proper PreMixing in-time. Messy, but possible. Probably blows the PreMix event size. |
|
I see, so the overlap at the SimHit level is important... that may require a custom "premix stub" format that stores more/different information (as we've done with HGCal etc.). Is there specific information that is needed vs. other information that can be discarded? (Even at the expense of some accuracy by making approximations) |
|
Would it blow the event size more than storing all the truth particles (which we're currently doing, but hope to disable by default by the end of the year)? |
|
I think the PreMix merging of the in-time hits is adequate as it is. For the out-of-time stubs, we just need counts and a momentum sorting. |
|
Not clear on event size. It would be some fraction of the Outer Tracker occupancy, multiplied by 8. |
|
Before we go down the road of trying to do this in production, I would like to make a hacked-up standalone version and see what the effects really are. We have some estimates from previous studies that the losses are relatively small. |
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
fully-signed
l1-approved
and removed
l1-pending
pending-signatures
labels
|
+1 |
This fixes several issues with simulation of truncation of stubs in the front-end tracker chips in the TTStubBuilder. (N.B. Until this PR, truncation has always been disabled by default, so these issues had no effect on official MC production).
N.B. At some point in the future, we should update further to take opto-link speed from DB.
PR validation:
Have created stubs in multithreaded job and run L1 tracking on them.