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

PF use raw ecal for PF hadron calibration #25703

Merged

Conversation

hatakeyamak
Copy link
Contributor

With this change, when the PF hadron calibration is called in PFAlgo, the raw ECAL energy will be used as inputs to pull the PF hadron calibration factor. Note that, when this calibration is derived, raw ECAL energy is used as inputs, and it allows the PF ECAL cluster calibration and PF hadron cluster calibration to be orthogonal.

The expected change (evaluated in a few MC samples):

  • effect in jet energy scale <1%, negligible in resolution (evaluated in QCD flat without PU) [1, page-4] [2] [3]
  • up to 10% reduction in PF photon energy and up to 1% reduction in PF neutral hadron energy (evaluated in neutrino gun MC with PU, L1 offset like observables) [1, page-6]

[1] https://indico.cern.ch/event/786078/contributions/3274894/attachments/1778982/2893356/PF_hadron_calib_20190115.pdf
[2] https://indico.cern.ch/event/777968/contributions/3255724/attachments/1773152/2882370/juska_pf_20181812.pdf
[3] https://indico.cern.ch/event/786078/contributions/3274894/attachments/1778982/2893550/juska_pf_20190115.pdf

@zdemirag @VinInn @cbernet @jpata @seemasharmafnal

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25703/8086

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @hatakeyamak (Kenichi Hatakeyama) for master.

It involves the following packages:

RecoParticleFlow/PFProducer

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @lgray, @seemasharmafnal, @bachtis, @cbernet 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 18, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 18, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32704/console Started: 2019/01/18 17:56

@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-25703/32704/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 10934 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3097440
  • DQMHistoTests: Total failures: 3401
  • DQMHistoTests: Total nulls: 12
  • DQMHistoTests: Total successes: 3093830
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.02 KiB( 31 files compared)
  • DQMHistoSizes: changed ( 136.731,... ): 0.008 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 1000.0 ): 0.004 KiB JetMET/SUSYDQM
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@hatakeyamak
Copy link
Contributor Author

I am not too familiar with checking outcomes of Jenkins tests...

Can I assume that this will be reviewed when people become available (in other words, there is no pending requests to me.) ?

Thank you.

// end loop on hcal element iHcal= hcalIs[i]
if ( fabs(eclusterref->energy() - sqrt(std::get<1>(ecalSatellite.second).Mag2()))>1e-3 || fabs(eclusterref->correctedEnergy() - ecalClusterEnergyCalibrated)>1e-3 )
edm::LogWarning("PFAlgo:processBlock") << "ecalCluster vs ecalSatellites look inconsistent (eCluster E, calibE, ecalSatellite E, calib E): "
<< eclusterref->energy() << " " << eclusterref->correctedEnergy() << " "
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the implications on the ecalSatellites data (or what follows from them)?
If the data is usable, this warning better be replaced by a LogInfo or put behind if (debug_.
If the data is completely unusable and/or if this simply is not supposed to happen (e.g. indicates a bug in the code), perhaps even an exception is a better solution.
@hatakeyamak please clarify

In my tests I did not see any instance of this happening out of about 7K events with 2018-like pileup.
So, at least from the point of view of frequency, this is OK to stay as a warning (as it does no harm so far).

@@ -2999,7 +3012,8 @@ void PFAlgo::processBlock( const reco::PFBlockRef& blockref,
reco::PFClusterRef eclusterRef = elements[iEcal].clusterRef();
assert( !eclusterRef.isNull() );

double ecalEnergy = eclusterRef->correctedEnergy();
// KH: use raw ECAL energy for PF hadron calibration.
double ecalEnergy = eclusterRef->energy(); // ecalEnergy = eclusterRef->correctedEnergy();
Copy link
Contributor

Choose a reason for hiding this comment

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

is the commented part of the code needed?
IIUC, there is no single place to make a switch from raw to calibrated.
So, the commented out part here can't be useful as a quick check to switch to the calibrated value.
Please drop the commented out // ecalEnergy = eclusterRef->correctedEnergy(); or add some note why this is still useful 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.

OK. I will clean up when I touch PFAlgo next time.

@slava77
Copy link
Contributor

slava77 commented Jan 26, 2019

+1

for #25703 9d6dd5f

  • code changes are in line with the PR description: the pfHadron energy calibration is now consistently using ECAL raw inputs
    • given that this is more correct from first principles, the following observations from the change are likely to be all considered as in the right direction
  • jenkins tests pass and comparisons with the baseline show mostly minor differences starting from the PF candidates and propagating downstream
  • local tests with more events confirm this behavior and also appear in line with what was shown in the PF group meeting (all changes are rather modest or small)
    • there is a smaller number of pf photons and neutral hadrons
    • additionally, the number of pf charged hadrons also goes down and the number of pf muons goes up
    • it looks like the overall sumEt is reducing by a fraction of one %.
    • other objects with significant changes in counts/distributions:
      • hps pi0 yields are down by a few %
      • lostTracks in miniAOD are up by a few % (mostly at low pt, below 1 GeV). My pre-expectations were that the tracks remain used; however, given the reduction even in the number of charged hadrons, quite a few really get rejected.

In event-by event comparison in workflow 136.862 ( EGamma2018B), in 200 events:

  • pf sumEt is mostly reduced: up in 2 events and down in 198 events
    • all of the 198 have the reduction in sumEt below 1%; 169 is above 0.1%; and 7 event above 0.5%

Some "representative" observations, here in ZEE events with PU (red is with this PR):
pf muons eta
all_sign1064vsorig_zee13tevpu2018wf11025p0c_recopfcandidates_particleflow__reco_obj_eta49
pf photon eta (counts drop, as expected)
all_sign1064vsorig_zee13tevpu2018wf11025p0c_recopfcandidates_particleflow__reco_obj_eta56
hps pi0 follow
all_sign1064vsorig_zee13tevpu2018wf11025p0c_recorecotaupizeros_hpspftauproducer_pizeros_reco_obj_eta
lost tracks
all_mini_sign1064vsorig_zee13tevpu2018wf11025p0c_log10patpackedcandidates_losttracks__reco_obj_pt
all_mini_sign1064vsorig_zee13tevpu2018wf11025p0c_patpackedcandidates_losttracks__reco_obj_eta

there is ever-so-small broadening (perhaps not stat significant; however this picture repeats in dijet and ZMM tests) in raw pfMet wrt GEN MET. This is probably fixable with type1 corrections, but that would need a new JEC.
wf11025_genmet

jet response in the dijet sample (flat with PU) is down a bit (as expected)
wf11071_ak4chs_resp_e

@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

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

4 participants