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

[HBHE] Always use 8 pulses in MAHI without 1-pulse to 8-pulse switch for Run3 and beyond #29617

Merged
merged 6 commits into from May 12, 2020

Conversation

jaehyeok
Copy link
Contributor

@jaehyeok jaehyeok commented May 2, 2020

MAHI first runs with 1 pulse that corresponds to SOI, and then moves to the fit with 8 pulses if the chi2 of the 1-pulse fit exceeds 15. The original intention of 1-pulse fit was to reconstruct high energy hits because the contribution from OOTPU is negligible, so using only 1 pulse was thought to be enough.

But, this had effects on low energy hits as well. For low energy hits the 8-pulse fit is more appropriate because the relative contribution from the OOTPU is large. However, in many cases 1-pulse fit was used instead of 8-pulse fit because the relative uncertainty of the fit is large, and this allows 1-pulse fit to have small chi2.

We studied the possibility of using 8-pulse fit only, without the 1-pulse to 8-pulse switch [1]. In the slides you can see that 8-pulse fit describes digi better than 1-pulse fit in both high and low energy regimes, so we want to start MAHI with 8 pulses. Same conclusion can be drawn from the study with PU [2].

This change is expected to impact on both data and MC.

This change is to be effective for Run3 and beyond only.

It passed runTheMatrix.py -l limited --ibeos.

[1] https://indico.cern.ch/event/913567/contributions/3841987/attachments/2029001/3395305/Mahi_Chi2Switch_for-Run3_29thApril.pdf
[2]
https://indico.cern.ch/event/911693/contributions/3857751/attachments/2035263/3407350/Simulation_Run3_PU.pdf

@cmsbuild cmsbuild changed the base branch from CMSSW_11_1_X to master May 2, 2020 01:43
@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2020

@jaehyeok, CMSSW_11_1_X branch is closed for direct updates. cms-bot is going to move this PR to master branch.
In future, please use cmssw master branch to submit your changes.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29617/14966

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2020

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

It involves the following packages:

RecoLocalCalo/HcalRecProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@apsallid, @mariadalfonso this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented May 2, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5957/console Started: 2020/05/02 08:16

@slava77
Copy link
Contributor

slava77 commented May 2, 2020

@jaehyeok
did I understand correctly that the study is based on 50 MC events with no PU for Run-3?

  • this doesn't seem to have a good coverage
    • without PU noise has a larger fraction of the total number of hits
    • the number of events seems too small
    • the dynamic range in ttbar is still too small to make conclusions for up to the max O(TeV) energy
  • shouldn't behavior with PU be checked?
  • what about Run-2 (phase-0) detector? IIUC, noise there is larger and noPU 8p is likely to reduce response to the intime pulse

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2020

+1
Tested at: 102745c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a7f53d/5957/summary.html
CMSSW: CMSSW_11_1_X_2020-05-01-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2020

Comparison job queued.

@mariadalfonso
Copy link
Contributor

mariadalfonso commented May 2, 2020

did I understand correctly that the study is based on 50 MC events with no PU for Run-3?

HCAL is not sensitive to the bunch spacing or soi as ECAL in configuration.
In fact no difference is seen for us between the epsilonPU and PU=0.
[q: where the 50ns is assumed other than the bunchspace = cms.int32(450) ?

  • this doesn't seem to have a good coverage

    • without PU noise has a larger fraction of the total number of hits
    • the number of events seems too small
    • the dynamic range in ttbar is still too small to make conclusions for up to the max O(TeV) energy
  • shouldn't behavior with PU be checked?

the current relVal show indeed that in presence of PU~200 is biased toward 1 pulse only

TTbar PU 2021 RelVal

  • more than ~40% is multipulses

TTabr PU200 2026D49

  • only ~10% is multiupulses
  • what about Run-2 (phase-0) detector? IIUC, noise there is larger and noPU 8p is likely to reduce response to the intime pulse

For Phase0 MC we should keep the old scheme:
for the HPD detector we do not have a good pulse shape and the multi-fit at high energy give a bad result. The switch in fact was designed for the M2 and we kept that to wait the all-SIPM detector (for which the pulse shape is measured directly with a phase scan).

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a7f53d/5957/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 30978 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2696239
  • DQMHistoTests: Total failures: 36521
  • DQMHistoTests: Total nulls: 29
  • DQMHistoTests: Total successes: 2659370
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.052 KiB( 33 files compared)
  • DQMHistoSizes: changed ( 1000.0,... ): -0.020 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 136.731 ): -0.008 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 136.793 ): -0.012 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 4.53 ): 0.008 KiB JetMET/SUSYDQM
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@abdoulline
Copy link

abdoulline commented May 2, 2020

@jaehyeok Jae,

(1) I've been asking to provide a link to (the ensemble of the MAHI-related presentations at) the meeting we held last Wednesday;
(2) would have been good to discuss the branch first within HCAL before submitting it,
mea culpa, I didn't ask it explicitly...
Maria has mentioned above that this changes is for >=Run3
(well, we did have Phase1 HE in 2018, which is a special case, but we may not want to opt for going back to 2018...)

I've come across this PR accidentally, not having been received any notification...

@slava77
Copy link
Contributor

slava77 commented May 2, 2020

did I understand correctly that the study is based on 50 MC events with no PU for Run-3?

HCAL is not sensitive to the bunch spacing or soi as ECAL in configuration.
In fact no difference is seen for us between the epsilonPU and PU=0.
[q: where the 50ns is assumed other than the bunchspace = cms.int32(450) ?

I didn't get this comment, as it was not a response to "did you test only on 50 events without PU?"
[I do not expect any difference between PU=0 and epsilonPU due to the way HCAL reco is set up. The difference in ECAL between PU=0 and PU=epsilon is purely from the way how reco is set up, not from how the PU is simulated.]

@slava77
Copy link
Contributor

slava77 commented May 2, 2020

Maria has mentioned above that this changes is for >=Run3
(well, we did have Phase1 HE in 2018, which is a special case, but we may not want to opt for going back to 2018...)

some relevant modifiers will need to be used.
We have run3_HB and run2_HE_2017 for detector-specific behavior settings. However, IIUC, the MAHI settings do not distinguish HB from HE, unless they are coupled in the code via hasTimeInfo.
From the description it sounds like this switch can be made for SIPMs and should not be made for HPDs. So, for a more targeted update some changes in the code will be needed.
A more covering way, this way applicable only to Run3 is to use (run3_HB & run2_HE_2017).toModify(mahiParameters, chiSqSwitch = -1) in HBHEMahiParameters_cfi

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@jaehyeok
Copy link
Contributor Author

@slava77 @Martin-Grunewald

Thanks very much! I just reverted it to the (no-era) simple customization.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29617/15285

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

Pull request #29617 was updated. @perrotta, @cmsbuild, @fwyzard, @Martin-Grunewald, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented May 11, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 11, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6229/console Started: 2020/05/11 18:05

@cmsbuild
Copy link
Contributor

+1
Tested at: 7677235
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a7f53d/6229/summary.html
CMSSW: CMSSW_11_1_X_2020-05-11-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a7f53d/6229/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 7647 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2697527
  • DQMHistoTests: Total failures: 21317
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2675891
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@slava77
Copy link
Contributor

slava77 commented May 11, 2020

+1

for #29617 7677235

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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 (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 07e298b into cms-sw:master May 12, 2020
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

10 participants