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

Remove tracker subdetector DetId in some Reco/Geometry ES products #19376

Merged
merged 2 commits into from Jun 22, 2017
Merged

Remove tracker subdetector DetId in some Reco/Geometry ES products #19376

merged 2 commits into from Jun 22, 2017

Conversation

pieterdavid
Copy link
Contributor

Purely technical change, no differences expected (part of the TIBDetId/TOBDetId/TIDDetId/TECDetID/PXBDetId/PXFDetId deprecation)

  • PFCheckHitPattern in RecoParticleFlow/PFTracking
  • SimpleDAFHitCollector in RecoTracker/SiTrackerMRHTools
  • TkGluedMeasurementDet in RecoTracker/MeasurementDet

cc @OlivierBondu @alesaggio @vidalm

@pieterdavid
Remove tracker subdetector DetId in some Reco/Geometry ESProducts
f7603f0 
- PFCheckHitPattern in RecoParticleFlow/PFTracking
- SimpleDAFHitCollector in RecoTracker/SiTrackerMRHTools
- TkGluedMeasurementDet in RecoTracker/MeasurementDet
@cmsbuild cmsbuild added this to the CMSSW_9_2_X milestone Jun 20, 2017
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @pieterdavid (Pieter David) for master.

It involves the following packages:

RecoParticleFlow/PFTracking
RecoTracker/MeasurementDet
RecoTracker/SiTrackerMRHTools

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @mmarionncern, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rafaellopesdesa, @rovere, @lgray, @mschrode, @bachtis, @cbernet, @gpetruc, @ebrondol, @VinInn, @dgulhan, @seemasharmafnal this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

makortel
makortel reviewed Jun 20, 2017
View reviewed changes
RecoParticleFlow/PFTracking/src/PFCheckHitPattern.cc Outdated
default:
throw cms::Exception("RecoParticleFlow", "Found DetId that is not in Tracker");
}
return DetInfo(subdetId, second);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this could be simplified to

return DetInfo(detId.subdetId(), tkerTopo->layer(detId));

Or maybe avoid this function altogether (AFAICT there is only a single call to this function)? e.g. in line 35

auto detId = dets[i]->geographicalId();
auto detInfo = DetInfo(detId.subdetId(), tkerTopo->layer(detId));

https://github.com/cms-sw/cmssw/blob/master/DataFormats/TrackerCommon/src/TrackerTopology.cc#L50-L67

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks for the suggestion, I pushed a new commit that implements it.

@pieterdavid
PFCheckHitPattern: use TrackerTopology::layer
a353537 
also removed PFCheckHitPattern::interpretDetId
@slava77
Copy link
Contributor

slava77 commented Jun 21, 2017 •
edited

@cmsbuild please test

strange that the bot didn't notice the last commit
@smuzaffar perhaps it's worth to check why

@slava77
Copy link
Contributor

slava77 commented Jun 21, 2017

@cmsbuild please test

[this time without a typo]

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 21, 2017 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20803/console Started: 2017/06/21 04:15

@cmsbuild
Copy link
Contributor

+1
Tested at: a353537
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19376/20803/summary.html

@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-19376/20803/summary.html

Comparison Summary:

  • You potentially added 5 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 22
  • DQMHistoTests: Total histograms compared: 1803263
  • DQMHistoTests: Total failures: 29852
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1773245
  • DQMHistoTests: Total skipped: 166
  • DQMHistoTests: Total Missing objects: 0
  • Checked 90 log files, 14 edm output root files, 22 DQM output files

@slava77
Copy link
Contributor

slava77 commented Jun 21, 2017

+1

for #19376 a353537

  • mostly technical; towards removal of dependence on specific tracker subdetector DetId classes
  • jenkins tests pass and comparisons show no differences

@cmsbuild
Copy link
Contributor

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@mmusich
Copy link
Contributor

mmusich commented Jun 21, 2017

tracked at #19398

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 1021dea into cms-sw:master Jun 22, 2017
@pieterdavid pieterdavid deleted the removeSiStripSubDetId_RecoGeometryESProducts branch September 28, 2018 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makortel makortel makortel left review comments

Assignees
No one assigned
Labels
comparison-available fully-signed orp-approved reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_9_2_X
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pieterdavid @cmsbuild @slava77 @mmusich @davidlange6 @makortel