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 (11_2_X) #34574

Merged
merged 18 commits into from Aug 10, 2021

Conversation

elusian
Copy link

@elusian elusian commented Jul 21, 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
  • bugfix from Remove the packedCandidate to gen associations in miniAOD_customizeAllData #34113

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 and #34113 for an Hackaton for GNN tracking

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 21, 2021

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

It involves the following packages:

  • Configuration/PyReleaseValidation (pdmv, upgrade)
  • Configuration/StandardSequences (operations)
  • PhysicsTools/PatAlgos (reconstruction)
  • SimTracker/Configuration (simulation)
  • SimTracker/TrackAssociation (simulation)
  • SimTracker/TrackerHitAssociation (simulation)

@perrotta, @civanch, @jordan-martins, @chayanit, @bbilin, @wajidalikhan, @kpedro88, @cmsbuild, @silviodonato, @srimanob, @mdhildreth, @kskovpen, @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, @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, @perrotta you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented Jul 22, 2021

please test

@slava77
Copy link
Contributor

slava77 commented Jul 26, 2021

backport of #33774

Copy link
Contributor

@jpata jpata left a comment

Choose a reason for hiding this comment

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

Could you comment if this PR was tested in a bParking workflow in 11_2_X (1304.182)? Thanks!


template <typename T>
std::vector<edm::DetSet<T>> pruneByTpAssoc(const edm::DetSetVector<T>& simLinkColl,
const std::set<std::pair<uint32_t, EncodedEventId>>& selectedIds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remind why std::set is used here, while std::unordered_set in master?
https://github.com/cms-sw/cmssw/blob/master/SimTracker/TrackAssociation/plugins/DigiSimLinkPruner.cc#L45

Copy link
Author

@elusian elusian Jul 27, 2021

Choose a reason for hiding this comment

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

The ability to use std::unordered_set comes from #33462 which added the UniqueSimTrackId type and its hasher that enables std::unordered_set. It is not available on 11_2_X as far as I know. I had to do the same modification when backporting to 10_6_X.
SimTracker/TrackerHitAssociation/plugins/ClusterTPAssociationProducer.cc for example shows the same difference between 11_2_X and master

@elusian
Copy link
Author

elusian commented Jul 27, 2021

Yes, this was tested on 1304.182

@jpata
Copy link
Contributor

jpata commented Jul 27, 2021

+reconstruction

@civanch
Copy link
Contributor

civanch commented Jul 27, 2021

+1

@srimanob
Copy link
Contributor

+Upgrade

@slava77
Copy link
Contributor

slava77 commented Aug 9, 2021

@cms-sw/pdmv-l2
please check this backport PR.
Thank you.

@kskovpen
Copy link
Contributor

kskovpen commented Aug 9, 2021

+1

@qliphy
Copy link
Contributor

qliphy commented Aug 10, 2021

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_11_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_1_X is complete. This pull request will be automatically merged.

@cmsbuild cmsbuild merged commit 52c5a4e into cms-sw:CMSSW_11_2_X Aug 10, 2021
@jpata
Copy link
Contributor

jpata commented Oct 1, 2021

This was merged a while ago, but is not in a release, as 11_2_X is closed. @elusian can you confirm that this is fine, or is something else required here?

@elusian
Copy link
Author

elusian commented Oct 1, 2021

This backport was for a Hackaton about GNN for tracking that concluded today. I believe the code was manually imported by the participants even if it was not in the release. No need to do anything else.

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

9 participants