cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.07k stars 4.28k forks source link

PFBlockAlgo cleanup #31385

Open jpata opened 4 years ago

jpata commented 4 years ago

The following technical refactoring for PFBlockAlgo can be considered:

  1. Unused and untested code path in SC linkers with SuperClusterMatchByRef=False: https://github.com/cms-sw/cmssw/blob/ec4c535880057c2e4d4d8b603f872bc9ea9eba43/RecoParticleFlow/PFProducer/plugins/linkers/SCAndECALLinker.cc#L45 https://github.com/cms-sw/cmssw/blob/ec4c535880057c2e4d4d8b603f872bc9ea9eba43/RecoParticleFlow/PFProducer/plugins/linkers/SCAndHGCalLinker.cc#L45

  2. Duplication or near-duplication of code for linking tracks and clusters via rechits: https://github.com/cms-sw/cmssw/blob/ec4c535880057c2e4d4d8b603f872bc9ea9eba43/RecoParticleFlow/PFProducer/plugins/kdtrees/KDTreeLinkerPSEcal.cc#L214 https://github.com/cms-sw/cmssw/blob/ec4c535880057c2e4d4d8b603f872bc9ea9eba43/RecoParticleFlow/PFProducer/plugins/kdtrees/KDTreeLinkerTrackEcal.cc#L221 https://github.com/cms-sw/cmssw/blob/14f85992a75e6bea754042de0d3f21de7e13cd51/RecoParticleFlow/PFClusterTools/src/LinkByRecHit.cc#L305

  3. useKDTree=False is not tested in test workflows for KDTreeLinkerPSEcal, KDTreeLinkerTrackEcal, KDTreeLinkerTrackHcal

jpata commented 4 years ago

assign reconstruction

attn @hatakeyamak @bendavid

cmsbuild commented 4 years ago

A new Issue was created by @jpata Joosep Pata.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

cmsbuild commented 4 years ago

New categories assigned: reconstruction

@slava77,@perrotta,@jpata you have been requested to review this Pull request/Issue and eventually sign? Thanks

jpata commented 3 years ago

Another thing to clean up:

HCALAndBREMLinker calls LinkByRecHit::testTrackAndClusterByRecHit, which always seems to return dist=-1 for HCAL and BREM. https://github.com/cms-sw/cmssw/blob/ec4c535880057c2e4d4d8b603f872bc9ea9eba43/RecoParticleFlow/PFProducer/plugins/linkers/HCALAndBREMLinker.cc#L38 https://github.com/cms-sw/cmssw/blob/ec4c535880057c2e4d4d8b603f872bc9ea9eba43/RecoParticleFlow/PFClusterTools/src/LinkByRecHit.cc#L88 https://github.com/cms-sw/cmssw/blob/ec4c535880057c2e4d4d8b603f872bc9ea9eba43/RecoParticleFlow/PFClusterTools/src/LinkByRecHit.cc#L125

slava77 commented 3 years ago

@laurenhay @marksan87 in case this is not already on your list

jpata commented 2 years ago

HCALAndBREMLinker calls LinkByRecHit::testTrackAndClusterByRecHit, which always seems to return dist=-1 for HCAL and BREM.

To add to the discussion at the PF meeting today, I'm not aware that this is a physics problem or a bug. Rather, I brought this up here as a possibility to simplify the linking code, and perhaps reduce the runtime, by avoiding ref lookups and track extrapolation if the answer for HCAL-BREM will always be -1 by construction. So I expect physics will change the same if you write

double HCALAndBREMLinker::testLink(const reco::PFBlockElement* elem1, const reco::PFBlockElement* elem2) const {
  return -1.0;
}

But if there are not many brems in an event, this could also have a very small effect on runtime overall (I haven't measured), so it may not be the highest-priority item to address.

jpata commented 2 years ago

type pf