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

Adding HBHE rechits to miniAOD #23540

Merged
merged 2 commits into from Jun 22, 2018

Conversation

Sam-Harper
Copy link
Contributor

In order to facilitate studies exploiting the updated HE depth segmentation, we would like to store a selection of interesting rec-hits around electron/photons in the miniAOD. This will make it easier to study how to use it and is a better option than randomly guessing and putting likely variables in the electron/photon.

This is done in the same way for ECAL rec hits.

Benchmark sample:
/RelValTTbar_13/CMSSW_10_2_0_pre4-PU25ns_102X_upgrade2018_realistic_v1-v1/GEN-SIM-RECO

cmsDriver command:
cmsDriver.py PAT -s PAT --datatier MINIAOD --runUnscheduled --nThreads 8 --mc --era Run2_2018 --scenario pp --conditions 102X_upgrade2018_realistic_v1 --eventcontent MINIAOD --filein file:/opt/ppd/month/harper/mcFiles/RelValTTbar_13__CMSSW_10_2_0_pre4__PU25ns_102X_upgrade2018_realistic_v1-v1__GEN-SIM-RECO__6E01D9DC-9861-E811-9D9B-0CC47A4D76A2.root --no_exec

using 1000 events.

from edmEventSize:

HBHERecHitsSorted_reducedEgamma_reducedHBHEHits_PAT. 1077.54 595.63
size per event  (compressed) 66475.7 -> 65956.4  (0.8% increase)

from "ls" a 0.8% increase

-rw-r--r-- 1 mjf21517 cms 69197272 Jun  8 14:19 miniAOD_withHCALRecHits.root
-rw-r--r-- 1 mjf21517 cms 68674012 Jun  8 14:33 miniAOD_withoutHCALRecHits.root

average number of HCAL hits stored : 20.9
image

I cleaned up the code a little in the interesting rec hit selector to avoid any copy pasting. Also we might have to reduce the HCAL thresholds (currently E>0.8 GeV), need to talk to HCAL to find what is sensible here.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2018

A new Pull Request was created by @Sam-Harper (Sam Harper) for master.

It involves the following packages:

PhysicsTools/PatAlgos
RecoEgamma/EgammaIsolationAlgos
RecoEgamma/EgammaPhotonProducers

@perrotta, @monttj, @cmsbuild, @slava77, @gpetruc, @arizzi can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @jainshilpi, @rappoccio, @varuns23, @seemasharmafnal, @mmarionncern, @imarches, @ahinzmann, @smoortga, @acaudron, @lgray, @jdolen, @ferencek, @rovere, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso, @pvmulder 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

@davidlange6
Copy link
Contributor

please test

@slava77
Copy link
Contributor

slava77 commented Jun 8, 2018

@arizzi @gpetruc
please check the added products/data and confirm that this increase is OK for miniAOD.
Thank you.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2018

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2018

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

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2902639
  • DQMHistoTests: Total failures: 26
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2902423
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 128 log files, 14 edm output root files, 31 DQM output files

{
DetId seedId = superClus.seed()->seed();
if(seedId.det() != DetId::Ecal && seedId.det() != DetId::Forward) {
edm::LogError("EgammaIsoHcalDetIdCollectionProducerError") << "Somehow the supercluster has a seed which is not ECAL, something is badly wrong";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should better return here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@perrotta
Copy link
Contributor

Pending items:

  • Please @Sam-Harper let us know when you have decided the appropriate HCAL thresholds, so that a more realistic evaluation of the additional size to miniAOD can be evaluated
  • Please @arizzi @gpetruc check the added products/data and confirm that the amount of size increase reported in the description of this PR is OK for miniAOD

@Sam-Harper
Copy link
Contributor Author

@perrotta So currently there are data/MC HCAL issues which is blocking 10_2. I think any study I do right now on the thresholds is likely rubbish. When this issue is resolved I think I can make a more meaningful study

@perrotta
Copy link
Contributor

@Sam-Harper : do you plan to have this integrated in 10_2? If so, the deadline for pre6 is ~tomorrow: to go on we need that you take #23540 (review) into account and that you tell us if you are fine with the current hcal thresholds or if you plan to tune them further now

@arizzi @gpetruc: we are still waiting for your evaluation of the miniAOD size increase

Of course, if you don't want to have it right now in 10_2 there is no hurry for all what above

@Sam-Harper
Copy link
Contributor Author

opps, pushed to the wrong branch only just noticed.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #23540 was updated. @perrotta, @monttj, @cmsbuild, @slava77, @gpetruc, @arizzi can you please check and sign again.

@davidlange6
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 21, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28807/console Started: 2018/06/21 22:43

@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-23540/28807/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 120 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2902147
  • DQMHistoTests: Total failures: 29
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2901928
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 128 log files, 14 edm output root files, 31 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Jun 22, 2018

Looking at the real events, as processed automatically by jenkins with wf 136.85 (RunEGamma2018A):

image

The reco event size increase can be perhaps considered in line with what reported for TTbar MC in #23540 (comment) (taking also into account that this is a EGamma stream and the peak at zero recHits as in the plot attached to the PR description is basically missing):

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
    815.6 ->      1904.4       1089     80.1   0.05     HBHERecHitsSorted_reducedHcalRecHits_hbhereco_reRECO.
-------------------------------------------------------------
  2327533 ->     2328612       1079             0.0     ALL BRANCHES

What puzzles me is what I get when I compare the miniAOD size:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
      0.0 ->      1661.9       1662     NEWO   3.46     HBHERecHitsSorted_reducedEgamma_reducedHBHEHits_reRECO.
-------------------------------------------------------------
    47987 ->       49649       1662             3.5     ALL BRANCHES

Weren't reducedHBHEHits allowed so far in 2018 miniAOD? If so, the overall miniOD event size increase there is quite relevant, and it should be at least re-evaluated...

Can you please comment, Sam?

@Sam-Harper
Copy link
Contributor Author

Reduced HBHE its is new for the reminiAOD this time around to facilitate studies on the new HCAL. Its nice but not mandatory, it will just make things a bit more difficult as we will have to AOD. Again a judgement call to whether it is worth it. For the AOD, its completely mandatory.

In EGamma, we'll always get an electron so we'll almost always get reduced rechits, the ttbar peak at zero was for non electron events. Its not clear these results are incompatible.

What I do know is that there is a data/MC missmodelling of low energy HCAL deposits (aka this) in the endcap and we as yet do not know which is "right".

@gpetruc
Copy link
Contributor

gpetruc commented Jun 22, 2018 via email

@perrotta
Copy link
Contributor

1661.9 is the the compressed size

The larger reduced RecHit multiplicity in EGamma events is obvious. What I wanted to know is whether the lack of reducedHBHEHits in baseline 10_2_X miniAOD is normal (answering myself: yes, as from https://github.com/cms-sw/cmssw/pull/23540/files#diff-969d6af05ee3397ef11cb0575b31a3abR35). If so, the number given by Sam for miniAOD TTbar in #23540 (comment) should read, instead:

HBHERecHitsSorted_reducedEgamma_reducedHBHEHits_PAT. 1734.43 951.211
which was
HBHERecHitsSorted_reducedEgamma_reducedHBHEHits_PAT.       0.         0.

(where the "which was" pointed out in that comment referred to the hypothetical situation in which the reducedHBHEHits would be allowed in the baseline miniAOD eventContent)

Did I understand correctly? If so: @arizzi @gpetruc do you confirm that the 950 bytes increase in the TTbar miniAOD event size is still acceptable to you?)

@Sam-Harper
Copy link
Contributor Author

Ah now I understand the problem, "which was" meant the value we with the old thresholds on this PR. The increase was always said to 950bytes. Sorry for not being clear

@gpetruc
Copy link
Contributor

gpetruc commented Jun 22, 2018 via email

@perrotta
Copy link
Contributor

perrotta commented Jun 22, 2018

+1

  • HBHE recHits around selected electrons/photons are added to the AOD and miniAOD output; reducedHBHEHits are also kept in the miniAOD output, which is not the case by now
  • Event size increase has been confirmed to be acceptable in both cases
  • Possible further tuning of the HCal thresholds (which can be obtained by modifying the corresponding parameters in the python config) can be postponed to a further update, if needed

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit ba63c27 into cms-sw:master Jun 22, 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

7 participants