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.

Sign up for GitHub

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

Jump to bottom

Better TrajectorySeed interface to recHits #29971

Merged
merged 4 commits into from Jun 5, 2020
Merged

Better TrajectorySeed interface to recHits #29971

merged 4 commits into from Jun 5, 2020

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented May 24, 2020 •
edited

PR description:

The current interface of the TrajectorySeed class to its recHits bothered me for quite some time, especially since the ElectronSeed inherits it. I'm speaking about this member function [1]:

std::pair<const_iterator, const_iterator> recHits() const { return std::make_pair(hits_.begin(), hits_.end()); }

which I think should better just return a const reference to the edm::OwnVector<TrackingRecHit> of seeds (proposed in this PR): original idea that got discarded in review
which should better return a edm::Range<RecHitContainer::const_iterator> that implements begin() and end() such that recHits() can be used in range-based loops:

edm::Range<RecHitContainer::const_iterator> recHits() const { return {hits_.begin(), hits_.end()}; }

this also avoids the very misleading expression recHits().second: one might think that it points to the second rechit, but actually it is the hits_.end() iterator. This caused possible bugs here [2] and here [3], I think. No attempt is made in this PR to fix these potential bugs, it only adapts the code to the newly proposed range-loop friendly interface without changing any functionality (hopefully).

[1] https://github.com/cms-sw/cmssw/blob/master/DataFormats/TrajectorySeed/interface/TrajectorySeed.h#L52
[2] https://github.com/cms-sw/cmssw/blob/master/RecoTracker/MeasurementDet/src/StartingLayerFinder.cc#L79
[3] https://github.com/cms-sw/cmssw/blob/master/Validation/RecoMuon/src/MuonSeedTrack.cc#L208

PR validation:

CMSSW compiles and local matrix tests pass.

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

No backport intended.

@cmsbuild cmsbuild added this to the CMSSW_11_2_X milestone May 24, 2020
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29971/15634

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

DPGAnalysis/SiStripTools
DQM/TrackingMonitor
DataFormats/EgammaReco
DataFormats/MuonSeed
DataFormats/ParticleFlowReco
DataFormats/TrajectorySeed
FastSimulation/Muons
FastSimulation/Tracking
Fireworks/Tracks
HLTrigger/Muon
RecoEgamma/EgammaElectronAlgos
RecoEgamma/Examples
RecoLocalTracker/SubCollectionProducers
RecoMuon/L2MuonSeedGenerator
RecoMuon/L3MuonProducer
RecoMuon/MuonSeedGenerator
RecoMuon/StandAloneTrackFinder
RecoMuon/TrackerSeedGenerator
RecoParticleFlow/PFSimProducer
RecoParticleFlow/PFTracking
RecoTracker/CkfPattern
RecoTracker/ConversionSeedGenerators
RecoTracker/FinalTrackSelectors
RecoTracker/MeasurementDet
RecoTracker/MkFit
RecoTracker/SingleTrackPattern
RecoTracker/TkSeedGenerator
RecoTracker/TrackProducer
SimMuon/MCTruth
SimTracker/TrackAssociatorProducers
TrackingTools/TrackRefitter
Validation/RecoMuon
Validation/RecoTrack

@perrotta, @Martin-Grunewald, @ssekmen, @Dr15Jones, @lveldere, @alja, @makortel, @civanch, @sbein, @schneiml, @andrius-k, @mdhildreth, @cmsbuild, @kmaeshima, @jfernan2, @fioriNTU, @slava77, @fwyzard, @santocch can you please review it and eventually sign? Thanks.
@wmtford, @echabert, @makortel, @felicepantaleo, @jainshilpi, @jandrea, @robervalwalsh, @echapon, @mschrode, @fioriNTU, @lgray, @abbiendi, @mmusich, @slomeo, @venturia, @mmarionncern, @hdelanno, @calderona, @pieterdavid, @threus, @jhgoh, @varuns23, @HuguesBrun, @seemasharmafnal, @yduhm, @rociovilar, @Sam-Harper, @cbernet, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @hatakeyamak, @alesaggio, @idebruyn, @ebrondol, @mtosi, @dgulhan, @Martin-Grunewald, @trocino, @battibass, @gbenelli, @Fedespring, @alja, @sobhatta, @lecriste, @afiqaize, @gpetruc, @matt-komm, @cericeci, @folguera this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@fwyzard
Copy link
Contributor

fwyzard commented May 24, 2020

Does the current code already guarantee that the rechits are always stored in a vector ?

@guitargeek
Copy link
Contributor Author

guitargeek commented May 25, 2020 •
edited

Pardon, I should have said that they are stored in an edm::OwnVector<TrackingRecHit> not a normal vector. I clarified that above. The OwnVectors interface fortunately comes with it's own iterator class and implements begin(), end() and empty(), which is why the range-based loop can be used:
https://github.com/cms-sw/cmssw/blob/master/DataFormats/Common/interface/OwnVector.h#L153
Unfortunately I'm not that familiar with the edm::OwnVector class, so if you have any further concerns I might not be able to comment them immediately.

@slava77
Copy link
Contributor

slava77 commented May 25, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 25, 2020 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6556/console Started: 2020/05/25 03:53

@cmsbuild
Copy link
Contributor

+1
Tested at: c6f9576
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ad7c88/6556/summary.html
CMSSW: CMSSW_11_2_X_2020-05-24-0000
SCRAM_ARCH: slc7_amd64_gcc820

@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-ad7c88/6556/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2780792
  • DQMHistoTests: Total failures: 4027
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2776715
  • DQMHistoTests: Total skipped: 50
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
  • Checked 152 log files, 16 edm output root files, 36 DQM output files

@jfernan2
Copy link
Contributor

+1

@Martin-Grunewald
Copy link
Contributor

+1

@silviodonato
Copy link
Contributor

Any objections?
analysis: @santocch
visualization: @Dr15Jones @alja @makortel
fastsim: @civanch @lveldere @mdhildreth @sbein @ssekmen
simulation: @civanch @mdhildreth

@santocch
Copy link

santocch commented Jun 3, 2020

+1

@alja
Copy link
Contributor

alja commented Jun 3, 2020

+1

@civanch
Copy link
Contributor

civanch commented Jun 5, 2020

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2020

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

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 8008927 into cms-sw:master Jun 5, 2020
@guitargeek guitargeek deleted the TrajectorySeed_recHits branch June 5, 2020 13:43
@guitargeek guitargeek mentioned this pull request Jun 7, 2020
Merged
@guitargeek guitargeek mentioned this pull request Jun 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perrotta perrotta perrotta left review comments

@fwyzard fwyzard fwyzard left review comments

Assignees
No one assigned
Labels
analysis-approved code-checks-approved comparison-available dqm-approved fastsim-approved fully-signed hlt-approved orp-approved reconstruction-approved simulation-approved tests-approved visualization-approved
Projects
None yet
Milestone
CMSSW_11_2_X
Development

Successfully merging this pull request may close these issues.

None yet