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

[Backport] Track-gen matching passing through TrackingParticle #33996

Merged
merged 18 commits into from Jun 13, 2021

Conversation

elusian
Copy link

@elusian elusian commented Jun 7, 2021

PR description:

This PR adds a way to match generalTracks (in AOD) and packedPFCandidates and lostTracks (in miniAOD) to a subset of genParticles, using association by hit on TrackingParticles.
The association is performed on step3 and just replicated in the miniAOD.
Since the track association by hit needs both TrackingParticles and SimLink for pixel and strips and the SimLink collections are not available in RAWSIM, two plugins are created to be run in step2 to add two pruned collections and avoid adding the all the SimLink, which have a non negligible size.

The changes contained are:

  • a plugin for pruning TrackingParticles in step2 based on their association to specific GenParticles. The genParticle selection is taken from the GenParticlePruner class and can be configured.
  • a plugin for pruning SimLink collections based on the SimTrack association to a TrackingParticle
  • a plugin for adding an association between PackedCandidates and GenParticles based on an existing association between tracks and GenParticles
  • Needed configuration changes to add this to standard sequences, gated behind the bParking modifier

No plugin is needed for association between tracks and GenParticles through TrackingParticle as it is already present in CMSSW (MCTrackMatcher).

PR validation:

On a B+->Psi2SK sample (one of the possible targets for this PR) we registered this increases in size, timing and memory

Size Timing Memory
RAWSIM +0.05% +0.08% +5MB
RECOSIM < +0.01% +0.07% +2MB
AODSIM +0.06%
MINIAODSIM +0.4% +0.08% +400kB

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

Backport of #33774 needed for BParking reprocessing

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2021

A new Pull Request was created by @elusian for CMSSW_10_6_X.

It involves the following packages:

Configuration/StandardSequences
PhysicsTools/PatAlgos
SimTracker/Configuration
SimTracker/TrackAssociation
SimTracker/TrackerHitAssociation

@perrotta, @civanch, @silviodonato, @mdhildreth, @cmsbuild, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@echabert, @rappoccio, @gouskos, @felicepantaleo, @wmtford, @robervalwalsh, @emilbols, @swozniewski, @Martin-Grunewald, @mbluj, @ahinzmann, @abbiendi, @mmusich, @seemasharmafnal, @mmarionncern, @makortel, @threus, @JanFSchulte, @jhgoh, @dgulhan, @jdolen, @prolay, @slomeo, @ferencek, @pieterdavid, @GiacomoSguazzoni, @rovere, @VinInn, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @ebrondol, @mtosi, @fabiocos, @clelange, @gbenelli, @JyothsnaKomaragiri, @hatakeyamak, @lecriste, @gpetruc, @mariadalfonso, @andrzejnovak this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented Jun 7, 2021

@cmsbuild, please test

@perrotta
Copy link
Contributor

perrotta commented Jun 7, 2021

backport of #33774

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2021

-1

Failed Tests: UnitTests RelVals RelVals-INPUT AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6facaa/15708/summary.html
COMMIT: bceee93
CMSSW: CMSSW_10_6_X_2021-06-06-2300/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33996/15708/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test runtestPhysicsToolsPatAlgos had ERRORS

RelVals

  • 5.15.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log
  • 4.534.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT/step2_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT.log
  • 4.224.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log
Expand to see more relval errors ...

RelVals-INPUT

  • 4.224.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log
  • 4.264.26_ZMuSkim2011A+ZMuSkim2011A+HLTDSKIM+RECODR1reHLT+HARVESTDR1reHLT/step2_ZMuSkim2011A+ZMuSkim2011A+HLTDSKIM+RECODR1reHLT+HARVESTDR1reHLT.log
  • 4.274.27_ZElSkim2011A+ZElSkim2011A+HLTDSKIM+RECODR1reHLT+HARVESTDR1reHLT/step2_ZElSkim2011A+ZElSkim2011A+HLTDSKIM+RECODR1reHLT+HARVESTDR1reHLT.log
Expand to see more relval errors ...

AddOn Tests

  • fastsimcmsDriver.py TTbar_8TeV_TuneCUETP8M1_cfi --conditions auto:run1_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot Realistic8TeVCollision : FAILED - time: date Mon Jun 7 12:51:05 2021-date Mon Jun 7 12:51:00 2021 s - exit: 256
  • hlt_data_Fake2cmsDriver.py RelVal -s L1REPACK:Full --data --scenario=pp -n 10 --conditions auto:run2_hlt_Fake2 --relval 9000,50 --datatier "RAW" --eventcontent RAW --customise=HLTrigger/Configuration/CustomConfigs.L1T --era Run2_2016 --fileout file:RelVal_Raw_Fake2_DATA.root --filein /store/data/Run2016B/JetHT/RAW/v1/000/272/762/00000/C666CDE2-E013-E611-B15A-02163E011DBE.root : FAILED - time: date Mon Jun 7 12:51:19 2021-date Mon Jun 7 12:51:08 2021 s - exit: 256
  • hlt_mc_Fake2
----- Begin Fatal Exception 07-Jun-2021 12:51:41 CEST-----------------------
An exception of category 'FileOpenError' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing input source of type PoolSource
   [2] Calling RootFileSequenceBase::initTheFile()
   [3] Calling StorageFactory::open()
   [4] Calling File::sysopen()
Exception Message:
Failed to open the file 'RelVal_Raw_Fake2_MC.root'
   Additional Info:
      [a] Input file file:RelVal_Raw_Fake2_MC.root could not be opened.
      [b] open() failed with system error 'No such file or directory' (error code 2)
----- End Fatal Exception -------------------------------------------------
Expand to see more addon errors ...

@@ -75,6 +76,10 @@
(pp_on_AA_2018 | _mAOD).toReplaceWith(slimmingTask,
slimmingTask.copyAndExclude([slimmedLowPtElectronsTask]))

from Configuration.Eras.Modifier_bParking_cff import bParking
_bParking_slimmingTask = cms.Task(slimmingTask.copy(),packedCandidateToGenAssociationTask)
bParking.roReplaceWith(slimmingTask,_bParking_slimmingTask)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bParking.roReplaceWith(slimmingTask,_bParking_slimmingTask)
bParking.toReplaceWith(slimmingTask,_bParking_slimmingTask)

@civanch
Copy link
Contributor

civanch commented Jun 11, 2021

@elusian , does this branch the same as for 12_0_X?

@elusian
Copy link
Author

elusian commented Jun 11, 2021

It's a backport. The additional changes should be only code format (not needed on master) and the activation using the bParking modifier

@qliphy
Copy link
Contributor

qliphy commented Jun 12, 2021

@cms-sw/simulation-l2 @cms-sw/pdmv-l2 @cms-sw/upgrade-l2 Do you have further comments?

@civanch
Copy link
Contributor

civanch commented Jun 12, 2021

+1

@srimanob
Copy link
Contributor

+Upgrade

@qliphy
Copy link
Contributor

qliphy commented Jun 13, 2021

+operations

@chayanit
Copy link

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_6_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_0_X is complete. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Jun 13, 2021

+1

@qliphy
Copy link
Contributor

qliphy commented Aug 28, 2021

@elusian @slava77 Sorry to come back to this. In 10_6_X IB, there is a reval error after merging this PR, can you have a look and comment? Is it expected similarly as mentioned in above, e.g.
#33996 (comment)
?

https://cmssdt.cern.ch/SDT/cgi-bin/logreader/slc7_amd64_gcc700/CMSSW_10_6_X_2021-08-27-2300/pyRelValMatrixLogs/run/10824.8_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFullINPUT+DigiFull_2018+RecoFull_ParkingBPH_2018+HARVESTFull_2018+ALCAFull_2018+NanoFull_2018/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFullINPUT+DigiFull_2018+RecoFull_ParkingBPH_2018+HARVESTFull_2018+ALCAFull_2018+NanoFull_2018.log#/

----- Begin Fatal Exception 28-Aug-2021 03:33:10 CEST-----------------------
An exception of category 'ProductNotFound' occurred while
[0] Processing Event run: 1 lumi: 122 event: 6057 stream: 3
[1] Running path 'MINIAODSIMoutput_step'
[2] Prefetching for module PoolOutputModule/'MINIAODSIMoutput'
[3] Prefetching for module PackedCandidateGenAssociationProducer/'lostTracksToGenAssociation'
[4] Prefetching for module MCTrackMatcher/'prunedTrackMCMatch'
[5] Prefetching for module QuickTrackAssociatorByHitsProducer/'quickPrunedTrackAssociatorByHits'
[6] Calling method for module ClusterTPAssociationProducer/'prunedTpClusterProducer'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: std::vector
Looking for module label: prunedTrackingParticles
Looking for productInstanceName:

Additional Info:
[a] If you wish to continue processing events after a ProductNotFound exception,
add "SkipEvent = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.

@elusian
Copy link
Author

elusian commented Aug 28, 2021

Hello
I tried running the 10824.8 workflow from the 10_6_X IB, and it seems that the problem is that step2 does not use the bParking modifier.
In this PR I added that modifier to another workflow (1304.181), but 10824.8 is not declared in Configuration/PyReleaseValidation/python/relval_standard.py and instead created step by step in Configuration/PyReleaseValidation/python/relval_upgrade.py.

I think this can be solved by adding 'Digi' to the steps for the ParkingBPH upgradeSteps, here https://github.com/cms-sw/cmssw/blob/CMSSW_10_6_X/Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py#L225 and modify the line here https://github.com/cms-sw/cmssw/blob/CMSSW_10_6_X/Configuration/PyReleaseValidation/python/relval_steps.py#L3300 to create the DigiFull_ParkingBPH_2018 step.

However, I'm not very familiar with that code so I would ask for a double check to avoid breaking something unrelated.
I'm rerunning the workflow in the meantime to see if it works with the modification

@elusian
Copy link
Author

elusian commented Aug 28, 2021

It seems that the fix works for this particular workflow, let me know if you agree with the changes or if I need to test anything else

@perrotta
Copy link
Contributor

It seems that the fix works for this particular workflow, let me know if you agree with the changes or if I need to test anything else

Thank you @elusian
The fix as you describe it makes sense: if the bParking modifier is not enabled in step2 something missing is likely for the following step3. Do you understand why it doesn't happen in the master?
Please prepare a PR with it, so that we can test it in the IB,

@elusian
Copy link
Author

elusian commented Aug 28, 2021

In master the PR is not gated behind a modifier, so it's enabled by default.
While master does not need a fix, should I make this change there first and then backport?

@perrotta
Copy link
Contributor

In master the PR is not gated behind a modifier, so it's enabled by default.
While master does not need a fix, should I make this change there first and then backport?

I am not 100% sure I understand the full logic. However, if master does not need a fix, I would only implement the fix where needed. The code in upgradeWorkflowComponents.py diverged significantly from 10_6 to the current one in the master, and it makes sense that corrections can be applied independently.
In any case, we are only touching a workflow which currently does not run in 10_6: any fix is welcome, and reconstruction and whatever else code will not get touched anyhow.

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