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

Include FSR for electrons (in addition to muons) #36323

Merged
merged 5 commits into from Jan 11, 2022

Conversation

vpicco
Copy link

@vpicco vpicco commented Dec 1, 2021

PR description:

As discussed in the XPOG meeting on Nov, 10, this PR adds:

  • FSR for electrons as well as for muons. A single collection of FsrPhotons is stored, with indexes "muonIdx" and "electronIdx" to the closest muon or electron. Either of the indexes is always -1. Note that (as it was before) more than one photon can in principle be matched to the same lepton, so that "masking" of different photons or "stealing" by other leptons can be addressed at analysis level if needed.
  • Electrons now have fsrPhotonIdx, like it was for muons.
  • FSR are collected down to the minimum lepton pT, and with no gap in the overlap region (was: pt_mu>20 and eta_gamma outside [1.4, 1.6])

PR validation:

Tested running on a RelVal sample and compared with the previous muon-only implementation.

Notes

Not sure of how PhysicsTools/NanoAOD/python/nanoDQM_cfi.py should be updated. Tried running prepareDQM.py, with no success. @mariadalfonso can you please advise on this?

@namapane pls follow

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36323/27062

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

A new Pull Request was created by @vpicco for master.

It involves the following packages:

  • CommonTools/RecoUtils (analysis)
  • PhysicsTools/NanoAOD (xpog)

@cmsbuild, @santocch, @mariadalfonso, @gouskos, @fgolf can you please review it and eventually sign? Thanks.
@gpetruc, @swertz this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@mariadalfonso
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a45494/20912/summary.html
COMMIT: 0e42c26
CMSSW: CMSSW_12_2_X_2021-12-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36323/20912/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 26 differences found in the comparisons
  • DQMHistoTests: Total files compared: 41
  • DQMHistoTests: Total histograms compared: 3041955
  • DQMHistoTests: Total failures: 41
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3041891
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 40 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 175 log files, 37 edm output root files, 41 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2021

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36323/27084

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36323/27087

@cmsbuild
Copy link
Contributor

Pull request #36323 was updated. @cmsbuild, @santocch, @mariadalfonso, @gouskos, @fgolf can you please check and sign again.

Comment on lines 142 to 143
double dR = deltaR(muon->eta(), muon->phi(), pc->eta(), pc->phi());
if (dR < dRMin && dR > 0.0001 && dR / pc->pt() / pc->pt() < drEtCut) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Event though also this one could get optimized without loosing readability, e.g..:

Suggested change
double dR = deltaR(muon->eta(), muon->phi(), pc->eta(), pc->phi());
if (dR < dRMin && dR > 0.0001 && dR / pc->pt() / pc->pt() < drEtCut) {
double dR = deltaR2(muon->eta(), muon->phi(), pc->eta(), pc->phi());
if (dR < dRMin * dRMin && dR > 0.0001 * 0.0001) {
dR = std::sqrt(dR);
if (dR < drEtCut * pc->pt() * pc->pt()) {

etc...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but this looks like a very tiny optimization, given that the number of candidates for FSR is small and the sqrt is done anyhow for those passing the dR cut (unless we also change the drEtCut). To me it seems to bring more confusion than speedup... but if you insist...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but this looks like a very tiny optimization, given that the number of candidates for FSR is small and the sqrt is done anyhow for those passing the dR cut (unless we also change the drEtCut). To me it seems to bring more confusion than speedup... but if you insist...

No, for this one I don't insist...

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36323/27702

@cmsbuild
Copy link
Contributor

Pull request #36323 was updated. @cmsbuild, @santocch, @mariadalfonso, @gouskos, @fgolf can you please check and sign again.

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a45494/21600/summary.html
COMMIT: 8546b67
CMSSW: CMSSW_12_3_X_2022-01-10-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36323/21600/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 28 differences found in the comparisons
  • DQMHistoTests: Total files compared: 43
  • DQMHistoTests: Total histograms compared: 3461659
  • DQMHistoTests: Total failures: 59
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3461577
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 42 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 181 log files, 42 edm output root files, 43 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

@cms-sw/xpog-l2 last updates were mostly code cleaning, and I think you can confirm your previous signature: please conferm it anyhow with your "+1", if so

@mariadalfonso
Copy link
Contributor

+xpog

quality code update since last signoff #36323 (comment)

@perrotta
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

merge
(bypassing analysis signature)

@cmsbuild cmsbuild merged commit 0c56c1e into cms-sw:master Jan 11, 2022
@santocch
Copy link

+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 be automatically merged.

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