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 SiPixel on track charge and shape probs to MiniAOD content #36247

Closed
wants to merge 15 commits into from

Conversation

tvami
Copy link
Contributor

@tvami tvami commented Nov 25, 2021

PR description:

See request to ReRECO 2017/2018 SM and MET data in [1]. This assumed refitting tracks to get the required low-level pixel reco information. As an alternative (actually it's an improvement, so better than alternative) it was recommended to save the required information in the MiniAOD. This PR does that, i.e. adds SiPixel on track charge and shape probs to MiniAOD content. Since in 2017/2018 the Pixel layer 1 was very noisy, and alternative version that excludes L1 was added as well.

  • Adding it to RECO/AOD as a new object siPixelTrackProbQXY:
edm::ValueMap<reco::SiPixelTrackProbQXY>    "siPixelTrackProbQXY"       ""        "RECO"
edm::ValueMap<reco::SiPixelTrackProbQXY>    "siPixelTrackProbQXY"       "NoLayer1"        "RECO"
  • While adding it to MiniAOD as a member of the isolatedTracks collection.

Further details presented in the Tracking POG meeting on Monday [2]

[1] https://its.cern.ch/jira/browse/PDMVRERECO-48
[2] https://indico.cern.ch/event/1098365/#5-proposal-to-use-low-level-pi

PR validation:

  • Running MiniAOD
cmsDriver.py ReRECO --conditions auto:run2_data --customise Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2018,Configuration/DataProcessing/Utils.addMonitoring --datatier MINIAOD --era Run2_2018,pf_badHcalMitigation --eventcontent MINIAOD --nThreads 8 --no_exec --number 100 --python_filename 4HLT2MiniAOD.py --scenario pp --step RAW2DIGI,L1Reco,RECO,EI,PAT --runUnscheduled --data  --filein root://cms-xrd-global.cern.ch//store/data/Run2018C/SingleMuon/RAW/v1/000/319/337/00000/004ED286-5582-E811-8895-FA163E126126.root --era Run2_2018

before the PR and after the PR the sizes change as follows:
-rw-r--r-- 1 tvami cms 8177333 Nov 24 16:30 ReRECO_RAW2DIGI_L1Reco_RECO_EI_PAT.root
-->
-rw-r--r-- 1 tvami cms 8185759 Nov 24 16:26 ReRECO_RAW2DIGI_L1Reco_RECO_EI_PAT.root
i.e an 1.00103041 increase in size is expected.

A unit test has been implemented as well

  • Running AOD
cmsDriver.py ReRECO --conditions auto:run2_data --customise Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2018 --datatier AOD --eventcontent AOD --number 100 --python_filename 4HLT2AOD.py --scenario pp --step RAW2DIGI,L1Reco,RECO --data  --filein file:/eos/cms/store/user/cmsbuild/store/data/Run2018C/SingleMuon/RAW/v1/000/319/337/00000/004ED286-5582-E811-8895-FA163E126126.root --fileout file:AOD.root --era Run2_2018

before the PR and after the PR the sizes of the AOD change as follows:
-rw-r--r--. 1 tvami zh 46456023 Dec 6 22:24 AOD.root
-->
-rw-r--r--. 1 tvami zh 47673431 Dec 6 22:23 AOD.root
i.e an 1.0262056 increase in size is expected.

Here is the timing report for before the PR

 Time Summary: 
 - Min event:   3.1252
 - Max event:   42.8974
 - Avg event:   12.985
 - Total loop:  181.876
 - Total init:  67.6784
 - Total job:   249.563
 - EventSetup Lock: 0
 - EventSetup Get:  0
 Event Throughput: 0.549825 ev/s
 CPU Summary: 
 - Total loop:     1176.02
 - Total init:     51.0267
 - Total extra:    0
 - Total children: 0.125992
 - Total job:      1227.06

and after the PR

 Time Summary: 
 - Min event:   3.22021
 - Max event:   41.7722
 - Avg event:   13.0204
 - Total loop:  180.607
 - Total init:  66.0196
 - Total job:   246.629
 - EventSetup Lock: 0
 - EventSetup Get:  0
 Event Throughput: 0.55369 ev/s
 CPU Summary: 
 - Total loop:     1183.12
 - Total init:     50.1868
 - Total extra:    0
 - Total children: 0.117669
 - Total job:      1233.31

Content was validated on MC Signal samples, and on data as well, please see more plots in [2]

if this PR is a backport please specify the original PR and why you need to backport that PR:

Needs a backport in 10_6_X

cc @mmusich

@tvami tvami marked this pull request as draft November 25, 2021 03:17
@tvami tvami changed the title Draft: Adding SiPixel on track charge and shape probs to MiniAOD content Adding SiPixel on track charge and shape probs to MiniAOD content Nov 25, 2021
@cmsbuild cmsbuild added this to the CMSSW_12_2_X milestone Nov 25, 2021
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36247/26887

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

@slava77
Copy link
Contributor

slava77 commented Nov 25, 2021

Code check has found code style and quality issues

in case it was not noticed already

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36247/26904

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @tvami (Tamas Vami) for master.

It involves the following packages:

  • DataFormats/PatCandidates (reconstruction)
  • DataFormats/TrackReco (reconstruction)
  • PhysicsTools/PatAlgos (reconstruction)
  • RecoTracker/Configuration (reconstruction)
  • RecoTracker/DeDx (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @felicepantaleo, @hatakeyamak, @emilbols, @mbluj, @demuller, @mmusich, @seemasharmafnal, @mmarionncern, @ahinzmann, @JanFSchulte, @dgulhan, @jdolen, @azotz, @GiacomoSguazzoni, @rovere, @VinInn, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @ebrondol, @mtosi, @mariadalfonso, @AlexDeMoor, @JyothsnaKomaragiri, @cbernet, @gpetruc, @andrzejnovak 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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36247/26906

@cmsbuild
Copy link
Contributor

Pull request #36247 was updated. @jpata, @cmsbuild, @slava77 can you please check and sign again.

@tvami tvami marked this pull request as ready for review November 25, 2021 18:43
@tvami
Copy link
Contributor Author

tvami commented Nov 25, 2021

@cmsbuild , please test

@tvami
Copy link
Contributor Author

tvami commented Dec 15, 2021

@smuzaffar tests have not come back in 24 hours, should re-trigger them?

@smuzaffar
Copy link
Contributor

looks like the profiling job is failing [a] , so re-triggering the test is not going to help.

[a]
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-profiling/146/

19:31:09 processing relval_upgrade
19:31:09 ERROR reading file: relval_upgrade 'HARVESTFakeHLT_trackingMkFit_2017'
19:31:09 Traceback (most recent call last):
19:31:09   File "/cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36247/21237/CMSSW_12_3_X_2021-12-13-1100/bin/slc7_amd64_gcc900/runTheMatrix.py", line 598, in <module>
19:31:09     ret = runSelected(opt)
19:31:09   File "/cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36247/21237/CMSSW_12_3_X_2021-12-13-1100/bin/slc7_amd64_gcc900/runTheMatrix.py", line 23, in runSelected
19:31:09     mrd.prepare(opt.useInput, opt.refRel, opt.fromScratch)
19:31:09   File "/cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36247/21237/CMSSW_12_3_X_2021-12-13-1100/python/Configuration/PyReleaseValidation/MatrixReader.py", line 506, in prepare
19:31:09     self.readMatrix(matrixFile, useInput, refRel, fromScratch)
19:31:09   File "/cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36247/21237/CMSSW_12_3_X_2021-12-13-1100/python/Configuration/PyReleaseValidation/MatrixReader.py", line 251, in readMatrix
19:31:09     if self.relvalModule.steps[stepName] is None:
19:31:09 KeyError: 'HARVESTFakeHLT_trackingMkFit_2017'

@mmusich
Copy link
Contributor

mmusich commented Dec 15, 2021

looks like the profiling job is failing [a] , so re-triggering the test is not going to help.

to solve this, PR #36477 is needed (see also #36347 (comment))

@tvami
Copy link
Contributor Author

tvami commented Dec 15, 2021

@cmsbuild , please abort

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36247/27386

@cmsbuild
Copy link
Contributor

Pull request #36247 was updated. @perrotta, @gouskos, @clacaputo, @fabiocos, @cmsbuild, @fgolf, @slava77, @jpata, @mariadalfonso, @qliphy, @davidlange6 can you please check and sign again.

@tvami
Copy link
Contributor Author

tvami commented Dec 15, 2021

@cmsbuild , please test with #36477

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ecf9e7/21289/summary.html
COMMIT: 75b2f0e
CMSSW: CMSSW_12_3_X_2021-12-15-1100/slc7_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36247/21289/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
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250719
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3250697
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor Author

tvami commented Dec 16, 2021

The idea from Slava to use the DeDxHitInfo's clusters and an approximation for the angles seems to be giving compatible results with my proposed solution. I'm removing the urgent label and will modify the PR so it keeps the producer only. That might still be useful for a possible future skim.

@cmsbuild cmsbuild removed the urgent label Dec 16, 2021
@tvami
Copy link
Contributor Author

tvami commented Dec 16, 2021

I'm closing this PR too, I'll open up a new one that will only contain the producer, maybe that actually could be attached to the skim development as well!

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