Small fix for simulation of pixel bad components #44276
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44276/39294
|
|
A new Pull Request was created by @ferencek for master. It involves the following packages:
@cmsbuild, @mdhildreth, @civanch can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
type bug-fix |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e6a164/37835/summary.html Comparison SummarySummary:
|
ferencek
changed the title
…mix stage2 The PixelFEDChannelCollection produced by the MixingModule in premix stage2 is not used at all so there is no point is creating it. Another PixelFEDChannelCollection is produced later by the DataMixer and propagated to tracking (after undergoing DigiToRaw and RawToDigi steps).
ferencek
force-pushed
the
premix_stage2_KillBadFEDChannels_bugfix
branch
from
92863e4 to
79272dc
Compare
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-approved
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44276/39310
|
|
Pull request #44276 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again. |
|
The PR title and description have been updated after doing some additional investigation. The good news is that there is no real bug in the simulation of pixel bad components. |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e6a164/37863/summary.html Comparison SummarySummary:
|
|
@ferencek , what is a bit worrying to me that explanation of History change due to this fix does not work for Run3 WFs - 2023 should be somehow affected. Instead only Run2 WF is affected. I agree that the effect is small for Run2 but am not sure about Run3. |
|
@civanch, do you maybe know right off the bat some 2023 WFs that should be affected so I could check in more detail. One thing that comes to my mind is that for Run 3 we don't yet have scenarios for transiently bad components included in any of the GTs (payloads are being prepared). |
|
+1 in that case, we may said that the PR is a simple fix, the effect of that will be seen later. |
cmsbuild
added
simulation-approved
fully-signed
and removed
simulation-pending
pending-signatures
labels
|
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. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2) |
|
@ferencek @cms-sw/simulation-l2 A backport is not needed here. |
|
+1 |
Is it? this has the simulation of transiently bad components. I haven't checked if it is used in any test workflow. If it doesn't it probably should. |
Is it worth it to open an issue? |
Somehow can be part of #41410 |
Indeed, this was queued back in August of last year https://cms-talk.web.cern.ch/t/new-pixel-status-scenarios-and-probabilities-for-2023-mc/28126/1 but I did not expect it to be used in any of the Run 3 wfs since there was no official campaign started yet that would use these payloads. In any case, the point of this PR was not to test status scenarios and probabilities but it unfortunately interferes with them by disabling one call to the random number generator which can then result in test differences which are nothing more than a red herring. |
On the contrary, development conditions should be tested as widely as possible before using them for large official campaigns to avoid surprises later on. I would urge the responsible groups to look into that. |
PR description:
This PR fixes a bug in the simulation of transiently bad components in the pixel detector which is at the moment enabled in both the MixingModule (configured here and here), where signal SimHits are digitized, and the DataMixer (configured here) where signal and pileup Digis are mixed together (they both have the
KillBadFEDChannelsswitch set toTruein theirSiPixelSimBlock). This PR sets this switch toFalsefor the MixingModule in thepremix_stage2modifier.PR validation:
No validation done. Impact on physics should be small in standard workflows. However, the impact should be more substantial for 2023 MC where there are sizable holes in pixel L3 and L4 in the second part of the year.
If this PR will be backported please specify to which release cycle the backport is meant for:
Backport to CMSSW_14_0_X needed to include this bugfix in Run 3 MC production
EDIT:
The severity of this PR luckily turns out to be a lot lower than it originally seemed. Since the
premix_stage2modifier already hasAddPixelInefficiencyset toFalsein this line, this switch also disables killing of transiently bad components in the MixingModule. In other words, there is no double killing of bad components (KillBadFEDChannels = Trueis a necessary but not sufficient condition to kill bad components. For this to happen,AddPixelInefficiencyalso needs to be set toTrue). Nevertheless, what theKillBadFEDChannelsswitch still controls is the creation of thePixelFEDChannelCollectionin the SiPixelDigitizer. Since this collection is not consumed by anything else, there is no point in wasting memory and computing cycles by producing it. Consequently, this PR is not critical for the Run 3 MC production so backporting it to CMSSW_14_0_X is strictly speaking not needed. Small differences observed in wf 250202.181 likely originate from a modified random number sequence (producing thePixelFEDChannelCollectionrequires calling a random number generator) which then results in some small differences in the final output.The commit message was also modified to better reflect the actual impact of the code change.