Skip to content
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

Kill both permanently and transiently bad pixel components in DataMixer #44320

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

ferencek
Copy link
Contributor

@ferencek ferencek commented Mar 5, 2024

PR description:

Up to now permanently bad components were killed prior to the DataMixer and separately for signal and pileup. For pileup this was happening in the MixingModule in stage 1 premixing and for signal in the MixingModule in stage 2 premixing. Instead, killing of transiently bad components was deferred to the last step in the DataMixer where signal and pileup digis are mixed. With this commit both permanently and transiently bad pixel components are killed in one place, in the DataMixer. This way premixed pileup library can be produced even before the list of bad components has been finalized and can later be reused (within the same production campaign) even with different bad component scenarios. The proposed change was presented in the Pixel Offline Meeting on Mar. 5, 2024 and the presentation can be found here.

PR validation:

Tested before and after the change using wf 250202.181 to confirm that permanently dead components are killed in both cases.

If this PR will be backported please specify to which release cycle the backport is meant for:

Backport to CMSSW_14_0_X is foreseen. Since this PR had a merge conflict with #44276, which had been resolved in the meantime, it might be best to backport both PRs together since they are somewhat related anyways.

@sroychow

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44320/39344

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2024

A new Pull Request was created by @ferencek for master.

It involves the following packages:

  • SimGeneral/MixingModule (simulation)
  • SimGeneral/PreMixingModule (simulation)

@civanch, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks.
@fabiocos, @slomeo, @makortel this is something you requested to watch as well.
@sextonkennedy, @rappoccio, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented Mar 5, 2024

For pileup this was happening in the MixingModule in stage 1 premixing and for signal in the MixingModule in stage 2 premixing.

Does this work as well for classical mixing?

@ferencek
Copy link
Contributor Author

ferencek commented Mar 5, 2024

For pileup this was happening in the MixingModule in stage 1 premixing and for signal in the MixingModule in stage 2 premixing.

Does this work as well for classical mixing?

Classical (full) mixing remains intact by this PR.

Up to now permanently bad components were killed prior to the DataMixer
and separately for signal and pileup. For pileup this was happening in
the MixingModule in stage 1 premixing and for signal in the MixingModule
in stage 2 premixing. Instead, killing of transiently bad components was
deferred to the last step in the DataMixer where signal and pileup digis
are mixed. With this commit both permanently and transiently bad pixel
components are killed in one place, in the DataMixer. This way premixed
pileup library can be produced even before the list of bad components
has been finalized and can later be reused with different bad component
scenarios.
@ferencek
Copy link
Contributor Author

ferencek commented Mar 5, 2024

Branch rebased to resolve conflict after #44276 got merged.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44320/39346

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2024

Pull request #44320 was updated. @civanch, @mdhildreth, @cmsbuild can you please check and sign again.

@civanch
Copy link
Contributor

civanch commented Mar 7, 2024

@ferencek , may be "draft" should be removed?

@ferencek
Copy link
Contributor Author

ferencek commented Mar 7, 2024

@ferencek , may be "draft" should be removed?

I agree. Done.

@ferencek ferencek marked this pull request as ready for review March 7, 2024 10:43
@sroychow
Copy link
Contributor

sroychow commented Mar 7, 2024

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 7, 2024

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-114bd4/37957/summary.html
COMMIT: cec8d84
CMSSW: CMSSW_14_1_X_2024-03-06-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/44320/37957/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 92 lines from the logs
  • Reco comparison results: 2805 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3293428
  • DQMHistoTests: Total failures: 4322
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3289086
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.011 KiB( 47 files compared)
  • DQMHistoSizes: changed ( 250202.181 ): -0.011 KiB SiStrip/MechanicalView
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: found differences in 1 / 46 workflows

@civanch
Copy link
Contributor

civanch commented Mar 9, 2024

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 9, 2024

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. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 07cf22f into cms-sw:master Mar 12, 2024
11 checks passed
@ferencek ferencek deleted the killModules_in_DataMixer branch March 12, 2024 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants