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

New thresholds for gathering and seeding in PF in EE, and deactivation of SR@PF #21846

Merged
merged 3 commits into from Jan 18, 2018

Conversation

amassiro
Copy link
Contributor

Deactivation of SR@PF for 2018.
80 MeV in EB and 300 MeV in EE in ZS regions is applied (negligible given the seeding and gathering thresholds, see below).

Update of PF ECAL cluster thresholds: seeding (500 MeV in E_T) and gathering (500 MeV in E_T) in EE.
EB thresholds for PF ECAL cluster are left untouched.

See the minutes of Fri 12th Jan meeting for details: https://indico.cern.ch/event/662757/attachments/1582189/2500613/EGM-ECAL_Minutes_120118.pdf

@argiro @crovelli

@cmsbuild cmsbuild changed the base branch from CMSSW_10_0_X to master January 12, 2018 21:15
@cmsbuild
Copy link
Contributor

@amassiro, CMSSW_10_0_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

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @amassiro (Andrea Massironi) for master.

It involves the following packages:

RecoParticleFlow/PFClusterProducer

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @felicepantaleo, @rovere, @lgray, @seemasharmafnal, @cbernet, @bachtis this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Jan 12, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 12, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25422/console Started: 2018/01/12 23:01

name = cms.string("PFRecHitQTestECALMultiThreshold"),
thresholds = particle_flow_zero_suppression_ECAL.thresholds
name = cms.string("PFRecHitQTestThreshold"),
threshold = cms.double(0.08)
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize that we have an issue in the barrel as well.
In case I missed something, please point me to some plots that quantify degradation in the barrel.

@amassiro
Copy link
Contributor Author

SR@PF issues also in the barrel had been raised by Tau POG, see for example the report at the PPD general meeting on the 14th Dec:

https://indico.cern.ch/event/685433/contributions/2810749/attachments/1575672/2488105/TauMisID_13122017_SRPF_PPD.pdf

@cmsbuild
Copy link
Contributor

@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-21846/25422/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-21846/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 12720 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2467710
  • DQMHistoTests: Total failures: 6221
  • DQMHistoTests: Total nulls: 12
  • DQMHistoTests: Total successes: 2461308
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.55 KiB( 22 files compared)
  • Checked 110 log files, 9 edm output root files, 26 DQM output files

@slava77
Copy link
Contributor

slava77 commented Jan 14, 2018

@amassiro @crovelli
I'd like to double-check that "Deactivation of SR@PF" still means that we pass all hits in the high interest region.
Recall that the 2016 behavior was to uniformly apply the same 0.08 or 0.3 GeV cut to all hits.
Your current change technically keeps the selective readout logic in place in PF (high interest region hits are all selected), it just relaxes/removes emulation of zero suppression thresholds offline.

seedingThreshold = cms.double(0.6),
seedingThresholdPt = cms.double(0.15)
seedingThreshold = cms.double(0.50),
seedingThresholdPt = cms.double(0.50)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have any Z->ee plots to show that this increase is OK?
I can imagine that shower or brem cases can degrade quite a bit.
@Sam-Harper what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

So we discussed this at length in the ECAL+E/gamma meeting with jetMET. JetMET need these thresholds to control the MET. For E/gamma, they are likely not ideal (although the results could be surprising with PU contributions), for the reasons you say. The plan is to check if it really causes a major (and it has to be major) problem for E/gamma although given the timeline, they may not complete in time for pre4 in which case we'll just go ahead.

If 10_0_0 has slightly worse performance in the endcap at low pt, its no big deal, we'll fix it next release. The samples we will need in 10_0_0 will be for energy regressions and we save the raw for those so in a pinch we can always re-reco them ourselves with new settings.

@slava77
Copy link
Contributor

slava77 commented Jan 16, 2018 via email

@slava77
Copy link
Contributor

slava77 commented Jan 16, 2018

@cmsbuild please test

in hope that 136.7611 file I/O error is transient.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 16, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25471/console Started: 2018/01/16 15:53

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@slava77
Copy link
Contributor

slava77 commented Jan 16, 2018

@amassiro @crovelli
please prepare a customise_command or a customise method to be able to go back to the previous SR@PF selections.
IIUC, this was needed for alternative relval submissions (@fabozzi ).

@cmsbuild
Copy link
Contributor

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-21846/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 6464 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2468799
  • DQMHistoTests: Total failures: 1580
  • DQMHistoTests: Total nulls: 3
  • DQMHistoTests: Total successes: 2467047
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.49999999988 KiB( 22 files compared)
  • Checked 110 log files, 9 edm output root files, 26 DQM output files

@argiro
Copy link
Contributor

argiro commented Jan 17, 2018 via email

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2018 via email

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2018

The last updates look more healthy for egamma:

e.g. 10 GeV photon gun
has a minor increase in the number of photons with overall OK quality
all_sign993-pass-f4f6008vsorig_singlegammapt10in2017wf10004p0c_recophotons_gedphotons__reco_obj_et
compare to the previous attempt in https://user-images.githubusercontent.com/4676718/34965595-0dd71d4c-fa0a-11e7-8458-0106b1ce31a7.png

The simulated (~empty detector) ECAL noise seems to stay under control, as seen e.g. in a mu gun 10 GeV
all_sign993-pass-f4f6008vsorig_singlemupt10in2017wf10007p0c_log10recopfmets_pfmet__reco_obj_pt
even though sumEt in this case goes up quite visibly by about 5 GeV or so.

Electrons look OK. In ZEE with PU there is some increase at low pt
all_sign993-pass-f4f6008vsorig_zee13tevpu2017wf10225p0c_recogsfelectrons_gedgsfelectrons__reco_obj_pt

jet response in the flat-pt spectrum also looks OK in the endcap (not much change in the barrel)
there is some increase in response
wf10071_f4f6008_ak4pf_e
and the histograms suggest that the resolution is slightly better esp in 20-40 GeV range
wf10071_f4f6008_ak4pf_20to40_e
wf10071_f4f6008_ak4pf_40to200_e
This is just a quick confirmation of expected behavior with this PR itself.

compare to the initial submission of this PR
https://user-images.githubusercontent.com/4676718/34965914-a0455156-fa0c-11e7-98b3-5c8b09578e2a.png

In my test of ZEE with PU 100 events MET looks OK (stats are low): a comparison with genMET
wf10225_f4f6008_pfmet

perhaps there is still a remaining issue for the rates with a significant MET as suggested in the previous discussion, but it's not visible in the bulk or in small stat tests.

Anyways, we are going back to the "good old" that we have used in 2016.

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2018

+1

for #21846 f4f6008

  • roll back PF ECAL hit selections to the logic used in 2016 (lowish thresholds are applied at hit level to all of the hits including the high interest regions and the regions with ZS are left to their online/emulated digi thresholds wherever they end up tighter than the "lowish thresholds". The opportunity remains open to adjust the baseline thresholds to be eta-dependent.)
  • jenkins tests pass and comparisons with baseline show differences that start from ecal PF and propagate downstream and affect 2017 and later workflows.
  • local tests with higher stats show that egamma response stays roughly the same and the jet response improves slightly in the endcap as expected.

@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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@amassiro
Copy link
Contributor Author

Hi Slava,

could you give us some reference on how to perform this? Where do we have to commit these fixes?

we can start with a list of "process.xyz = zyt" python statements needed
to go back to the default.
This is needed only in case the "SR@PF ON" relval will be requested.

Cheers,
Andrea, Chiara and Stefano

@crovelli @argiro @slava77

@fabozzi
Copy link
Contributor

fabozzi commented Jan 17, 2018

@crovelli @argiro @slava77 @amassiro
The question is: is it possible to revert back to "SR@PF ON" with a list of statements which modify at cfg level the relevant parameters? If so, this will ease "SR@PF ON" relval production because we won't need to set up and recompile a modified local CMSSW area and write down the full RECO cfg
(possible, but more painful).

Instead, if it is just a matter of changing cfg parameters of the process (i.e. the list of statements "process.xyz = zyt") we can try to do that on the fly, with a customise_command in the cmsDriver of the relval steps. You do not need to commit anything, just list the needed statements.

@fabiocos
Copy link
Contributor

@fabozzi we need to follow up on this point, but I suggest this is done in a separate PR. I now integrate the present one.

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 64521b3 into cms-sw:master Jan 18, 2018
@silviodonato silviodonato mentioned this pull request Jan 25, 2018
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