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

HCAL: make HBHE zero suppression simulation identical to the data-taking one for Run3 #40444

Merged
merged 2 commits into from Jan 9, 2023

Conversation

abdoulline
Copy link

@abdoulline abdoulline commented Jan 7, 2023

PR description:

It's a trivial fix (of long-lasting omission)
to make HBHE ZS simulation identical to what's actually in the HCAL firmware at P5 since early 2022...
The effect on 2022 MC (with yet fairly low DC noise) is small, see default 2022 single-pion (noPU) comparison w/wo this PR
https://cms-docs.web.cern.ch/hcal-sw-validation/calo_scan_single_pi/13_0_X/13_0_0_pre2_run3_ZS_SOI_vs_13_0_0_pre2_run3_SinglePi/

A priori expectations for
(1) regular Run3 wfs: decrease of the number of very low-e hits (<50-100 MeV in RecHits energy equivalent)
naturally PU wfs are somewhat more affected than noPU ones;
(2) Phase2 wfs without aging: similar to (1);
(3) Phase2 wfs with the regular 1000/fb aging: no changes .

PR validation:

runTheMatrix.py -l limited

If this PR is a backport

No. But for any 2023/2024-related MC studies using 12_6_X with (projected) high DC noise conditions, a backport #40447 would be needed.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40444/33596

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2023

A new Pull Request was created by @abdoulline (Salavat Abdullin) for master.

It involves the following packages:

  • SimCalorimetry/HcalZeroSuppressionProducers (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@bsunanda, @sameasy, @rovere this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@civanch
Copy link
Contributor

civanch commented Jan 7, 2023

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15b519/29824/summary.html
COMMIT: 4537ef6
CMSSW: CMSSW_13_0_X_2023-01-07-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40444/29824/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4573 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555748
  • DQMHistoTests: Total failures: 3722
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3552004
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: found differences in 3 / 47 workflows

@civanch
Copy link
Contributor

civanch commented Jan 7, 2023

+1

@abdoulline , I would expect that regression can be slightly modified

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2023

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

@abdoulline
Copy link
Author

@abdoulline , I would expect that regression can be slightly modified

@civanch I'm not sure I understand your question, could you, please re-formulate it in a more explicit way?

Copy link
Contributor

@perrotta perrotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdoulline , perhaps while waiting for @civanch explaining his comment you could clean this config by removing all type specifications for cloned parameters, here and also in the modifications applied above in this file

@abdoulline
Copy link
Author

@abdoulline , perhaps while waiting for @civanch explaining his comment you could clean this config by removing all type specifications for cloned parameters, here and also in the modifications applied above in this file

@perrotta good point, done.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40444/33599

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2023

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

@perrotta
Copy link
Contributor

perrotta commented Jan 8, 2023

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15b519/29829/summary.html
COMMIT: bcc106c
CMSSW: CMSSW_13_0_X_2023-01-08-0000/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40444/29829/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4815 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555748
  • DQMHistoTests: Total failures: 3165
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3552561
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: found differences in 3 / 47 workflows

@abdoulline
Copy link
Author

RecHits energy spectra from the most affected wf 11834.0 (Run3 TTbar PU)

HB
https://tinyurl.com/yz6pa7wn

HE
https://tinyurl.com/y9w2k5nu

@civanch
Copy link
Contributor

civanch commented Jan 9, 2023

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2023

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

@civanch
Copy link
Contributor

civanch commented Jan 9, 2023

@abdoulline , sorry, I mean that this PR should slightly modify few plots and this is OK.

@perrotta
Copy link
Contributor

perrotta commented Jan 9, 2023

+1

@cmsbuild cmsbuild merged commit 44c3f64 into cms-sw:master Jan 9, 2023
@srimanob
Copy link
Contributor

Hi @abdoulline

Looking on PR description, you said
(2) Phase2 wfs without aging: similar to (1);
(3) Phase2 wfs with the regular 1000/fb aging: no changes .

Do I understand correctly that
(2) ==> Seen in PR test
(3) ==> We can't check until we have release
?

@abdoulline
Copy link
Author

Hi @srimanob
that's correct, I believe.
For (3) pedestals are so high that actual ZS cuts are ineffective no matter w/wo this fix.
There is and will be 100% HB occupancy in Phase2 wfs with aging1000/fb...

@srimanob
Copy link
Contributor

Hi @abdoulline
OK, thanks. Actually, we have a wf with aging1000/fb which can be tested with PR test? Should I do that for the open one, just to make sure everything is as expected.

@abdoulline
Copy link
Author

@srimanob yes, I know, more so -it's easy/straightforward to add aging customization to any Phase2 wf (cmsDriver).
I'd appreciate if you could make this test, as I'm quite busy now with HCAL preparations for TSG studies...

@abdoulline abdoulline deleted the HBHE_ZS_1TS_SOI branch April 18, 2023 05:36
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.

None yet

5 participants