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

Add electrons from HGCAL in the pfCandidate collection #20614

Merged

Conversation

rovere
Copy link
Contributor

@rovere rovere commented Sep 21, 2017

The previously saved collection is created before merging the Barrel and HGCAL-related, sim-driven, components together. The newly saved collection will contain all pfcandidates that should be used to perform physics studies.
This will only affect PhaseII workflows. I expect no regression and very tiny effect on the event size of the final ROOT file.

Also, electrons built within the HGCAL volume were not saved into the particleFlow pfCandidate collection, causing strange effects (artificial MET, missing JETs etc...). Now they are forced into the final collection.

@malgeri, @cseez, @felicepantaleo @kpedro88

A backport to 93X branch is imminent.

@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/PR-20614/881

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @rovere (Marco Rovere) for master.

It involves the following packages:

RecoParticleFlow/Configuration

@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 you are the release manager for this.

cms-bot commands are listed here

@kpedro88
Copy link
Contributor

assign upgrade

@kpedro88
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 21, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23147/console Started: 2017/09/21 17:26

@@ -156,7 +156,7 @@ def _modifyPFEventContentForHGCalFEVT( obj ):
'keep recoPFClusters_particleFlowClusterHGCalFromMultiCl__*',
'keep *_particleFlowSuperClusterHGCalFromMultiCl_*_*',
'keep *_simPFProducer_*_*',
'keep *_particleFlowTmpBarrel_*_*',
'keep *_particleFlowTmp_*_*'
Copy link
Contributor

Choose a reason for hiding this comment

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

process.particleFlowTmp = cms.EDProducer("PFCandidateListMerger",
    src = cms.VInputTag("particleFlowTmpBarrel", "simPFProducer")
)

Is the content of _particleFlow_ inappropriate (it is in the standard event content already)?
It's made out of particleFlowTmp. So, this would just add a copy of the particleFlow collection with pre-linking step information about objects and links.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rovere
please clarify on this.
It appears to me that saving the Tmp for the merged pf collection is not needed because the relinked collection contains all of its information

Copy link
Contributor

Choose a reason for hiding this comment

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

@rovere
I'm waiting for feedback on this. (what's missing in particleFlow to require also saving particleFlowTmp?)
Please comment.

Copy link
Contributor Author

@rovere rovere Sep 26, 2017

Choose a reason for hiding this comment

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

@slava77 most likely, after the 2 additional commits that store the electrons directly into pfCandidates, the particleFlorTmp could be dropped completely.

@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-20614/23147/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2654399
  • DQMHistoTests: Total failures: 214
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2653996
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@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/PR-20614/973

@cmsbuild
Copy link
Contributor

Pull request #20614 was updated. @perrotta, @cmsbuild, @kpedro88, @slava77 can you please check and sign again.

@davidlange6
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 26, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23240/console Started: 2017/09/27 00: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-20614/23240/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 679 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2691683
  • DQMHistoTests: Total failures: 296
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2691198
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 15 edm output root files, 26 DQM output files

@slava77
Copy link
Contributor

slava77 commented Sep 27, 2017

PU200:
there is a 20-fold increase in the number of PF electrons

all_sign968-2023pu200vsorig-2023pu200_ttbar14tev2023d17putimingwf20234p2c_log10recopfcandidates_particleflow__reco_obj_p54

essentially all are with pt below about 3 GeV and are in the endcap
all_sign968-2023pu200vsorig-2023pu200_ttbar14tev2023d17putimingwf20234p2c_log10recopfcandidates_particleflow__reco_obj_pt53

The purity is apparently rather poor, but since it's mostly all at low pt, it seems rather harmless.
I suppose, bad purity is still better than zero efficiency/acceptance.

Considering that all pf electrons in the endcap are from sim truth match to generalTracks, the purity should actually be high.

in DQM, jet, met, and btags look about unchanged apart for the electron fraction or similar plots.

@slava77
Copy link
Contributor

slava77 commented Sep 27, 2017

+1

for #20614 c55b98d

  • non-ged (HGCAL part in this case) electrons that made it to the PF merged "tmp" (pre-linked) collection via the PFSim producer are patched into the final particleFlow collection by dropping a match requirement to a GED electron; affects only phase2
  • jenkins tests pass and comparisons with baseline show differences only in phase-2 workflows, starting from PF candidate distributions.
  • local test with PU200 show appearance of pf electrons in the endcaps, almost all are at low PT (perhaps secondaries, still from actual electrons due to sim truth nature of ele ID assignment in SimPFProducer).
    • CPU, memory and disk impact is small (the number of added pf electrons compared to all other PF candidates is very small)

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 897a3cf into cms-sw:master Sep 27, 2017
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