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 PFClusterIsolation to reco Egamma objects #21811

Merged
merged 3 commits into from Feb 6, 2018

Conversation

Sam-Harper
Copy link
Contributor

PFCluster isolation is used in the HLT and is becoming more popular in E/gamma as useful isolation.

It is already stored in AOD via value maps and is embedded into pat::Electrons and pat::Photons. This change puts it directly in the reco::GsfElectron and reco::Photon for ease of use. It should be CPU and diskspace neutral (if anything, slightly less diskspace as it replaces valuemaps) as its simply changing where the isolation value is stored.

The value maps are now added to the electron,photon during the finalisation step in the same way as PFIsolations are. As such the event content is modified to drop these ValueMaps as they are unnecessary (and keyed to the tmp versions of the electrons/photons). The reducedEgamma and pat configs were updated to not to store the PF cluster isolation. However they still have a capability to read in value maps and set the PFCluster isolation. This is done incase we wish to use this release to re-miniAOD 8X or 9X samples where a customisation function would configure these modules to read the value maps. Note, I wasnt sure how to do such a customisation function so any advice here would be welcome.

The only slightly tricky part was with the OOT photons which dont have PF isolation and so dont have a finalisation step. I added one for them and they should be untouched otherwise. @kmcdermo FYI

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2018

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

It involves the following packages:

DataFormats/EgammaCandidates
DataFormats/PatCandidates
PhysicsTools/PatAlgos
RecoEgamma/Configuration
RecoEgamma/EgammaElectronProducers
RecoEgamma/EgammaIsolationAlgos
RecoEgamma/EgammaPhotonProducers

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

@slava77
Copy link
Contributor

slava77 commented Jan 8, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25327/console Started: 2018/01/08 15:54


ootPhotons = _gedPhotons.clone(
photonProducer = 'ootPhotonsTmp',
candidateP4type = "fromEcalEnergy",
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sam-Harper , checking https://github.com/Sam-Harper/cmssw/blob/6e6a25fd65e53bbf5a43d99fecab8a2f46712896/RecoEgamma/EgammaPhotonProducers/src/GEDPhotonProducer.cc#L841-L843, this means that the regression is effectively skipped in the final step for ootPhotons, correct?

I guess the nice thing here is that, now the symmetry between ged and oot photons is closer with a tmp and final step for both, even if nothing really happens in the final step for oot photons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the intention was to skip the regression for OOT photons.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2018

-1

Tested at: 6e6a25f

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21811/25327/summary.html

I found follow errors while testing this PR

Failed tests: RelVals AddOn

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
136.7611 step2

runTheMatrix-results/136.7611_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM/step2_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM.log

140.53 step2
runTheMatrix-results/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log

  • AddOn:

I found errors in the following addon tests:

cmsDriver.py RelVal -s HLT:HIon,RAW2DIGI,L1Reco,RECO --data --scenario=HeavyIons -n 10 --conditions auto:run2_data_HIon --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_2016,Run2_HI --processName=HLTRECO --filein file:RelVal_Raw_HIon_DATA.root --fileout file:RelVal_Raw_HIon_DATA_HLT_RECO.root : FAILED - time: date Mon Jan 8 18:19:37 2018-date Mon Jan 8 18:13:01 2018 s - exit: 16640
cmsDriver.py RelVal -s HLT:HIon,RAW2DIGI,L1Reco,RECO --mc --scenario=HeavyIons -n 10 --conditions auto:run2_mc_HIon --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_HI --processName=HLTRECO --filein file:RelVal_Raw_HIon_MC.root --fileout file:RelVal_Raw_HIon_MC_HLT_RECO.root : FAILED - time: date Mon Jan 8 18:24:37 2018-date Mon Jan 8 18:17:31 2018 s - exit: 16640

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2018

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@Sam-Harper
Copy link
Contributor Author

thanks Andrea and Slava, I've updated the name now

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #21811 was updated. @perrotta, @monttj, @cmsbuild, @franzoni, @slava77, @fabiocos, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 31, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25803/console Started: 2018/01/31 15:58

@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-21811/25803/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-21811/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 50 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2463035
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2462865
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.00000000002 KiB( 22 files compared)
  • Checked 110 log files, 9 edm output root files, 26 DQM output files

@perrotta
Copy link
Contributor

+1

  • ecal/hcalPFClusterIso() are filled for ged electrons/photons
  • output sizes do not increase: in fact the reco output even tinily shrinks, thanks to the removal of the floatedmValueMap's (as anticipated in the PR description)
  • latest jenkins tests show 50 differences in the comparisons: this is simply due to the introduction of those ecal/hcalPFClusterIso() in the monitored quantities for miniAOD. No other differences are observed, as expected

@slava77
Copy link
Contributor

slava77 commented Feb 2, 2018

@fabiocos
Please clarify your plans for this PR.
This contains a new modifier run2_miniAOD_94XFall17 which will be needed for re-miniAOD PRs and backports. If this PR will take longer than until Monday, then perhaps the feature introducing the new modifier will need to be split off into a separate PR (assuming that it can go in quickly).

@fabiocos
Copy link
Contributor

fabiocos commented Feb 6, 2018

+operations

@fabiocos
Copy link
Contributor

fabiocos commented Feb 6, 2018

+1

@fabiocos
Copy link
Contributor

fabiocos commented Feb 6, 2018

merge

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

8 participants