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

Allow ECAL pfrechit thresholds eta dependent in all regions, disregarding if ZS of FR (10-1-X) #22265

Merged

Conversation

amassiro
Copy link
Contributor

Allow ECAL pfrechit thresholds eta dependent in all regions, disregarding if they have been read in Zero Suppression (ZS) of Full Readout (FR).

In the previous version we could activate these thresholds only in ZS regions, or de-activate the application of the thresholds everywhere.

With this PR we can activate the thresholds everywhere, by means of the boolean: "applySelectionsToAllCrystals"

HLT customization added as well, given the new parameter added in the pfrechit producer..

In addition, definition of reasonable thresholds that can be used by jetmet/HLT/EGamma/Tau POGs to estimate possible changes in pf-rechit thresholds to adapt to 2018 data-taking conditions.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@silviodonato
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 19, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26167/console Started: 2018/02/19 15:49

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

HLTrigger/Configuration
RecoParticleFlow/PFClusterProducer

@perrotta, @cmsbuild, @silviodonato, @slava77, @Martin-Grunewald, @fwyzard can you please review it and eventually sign? Thanks.
@mmarionncern, @makortel, @felicepantaleo, @rovere, @lgray, @Martin-Grunewald, @seemasharmafnal, @jalimena, @cbernet, @geoff-smith, @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

# CMSSW version specific customizations
def customizeHLTforCMSSW(process, menuType="GRun"):

# add call to action function in proper order: newest last!
# process = customiseFor12718(process)

process = customiseForEcalTestPR22254Default(process)
process = customiseFor21821(process)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please put the calls in order: most recent last!

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Feb 20, 2018

@amassiro
please clarify if you were going to update the values to match closer the 2016 thresholds now
or if it's something you will consider for later.

I'm not particularly attached to the old values, but if the new proposed ones are only 10-20% different, the justification for the change is likely rather weak. (as e.g. EB case C)

@slava77
Copy link
Contributor

slava77 commented Feb 20, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 20, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26201/console Started: 2018/02/20 19:06

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@amassiro
Copy link
Contributor Author

We do not plan to update w.r.t. what is currently committed.
The values that have been commited on github have been already shown and discussed among POGs representatives. The values are meant to be used for testing purposes but not for actual data-taking, unless there is a request by POGs and PAGs. The default value is the same as in the previous cmssw release (80 MeV and 300 MeV).

On the technical side, we are working on a DB interface for these values, so that we don't need to go through the explicit definition in cmssw, but it is not ready yet, while we would like to have this included so that people can already test the effect of different thresholds on objects performance and HLT.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 28
  • DQMHistoTests: Total histograms compared: 2498032
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2497855
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.829999999893 KiB( 23 files compared)
  • Checked 115 log files, 9 edm output root files, 28 DQM output files

@slava77
Copy link
Contributor

slava77 commented Feb 20, 2018 via email

@slava77
Copy link
Contributor

slava77 commented Feb 20, 2018

+1

for #22265 c31cddc

  • bugfix to a latent bug (not visible in the default configuration): thresholds on PF ECAL hit loading should be applied to all crystals in case srFlag is not used.
  • jenkins tests pass and comparisons with baseline show no differences
    • I tested earlier somewhat equivalent changes in the code and saved recoPFRecHits_particleFlowRecHitECAL__ to confirm the changes in this collection are as expected.

@fabiocos
Copy link
Contributor

@Martin-Grunewald I assume you are still ok with this PR, could you please sign it again before I merge it as requested?

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

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 92b84c4 into cms-sw:master Feb 21, 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

6 participants