New TrackingRecHitRange and use it in RecoParticleFlow #25591

guitargeek · 2019-01-07T10:05:49Z

This PR suggests to add a member function to TrackExtraBase which returns an instance of the new TrackingRecHitRange (template instantiation of new edm::ConstRange) that can be used to express range-based loops over the RecHits associated to a track. I think this would be beneficial for the modernization of reconstruction code, as I show at the example of RecoParticleFlow, where I replaced all occurrences of recHitsBegin and recHitsEnd.

Local matrix tests pass.

cmsbuild · 2019-01-07T10:06:17Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-01-07T10:12:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25591/7835

cmsbuild · 2019-01-07T10:14:04Z

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

It involves the following packages:

DataFormats/CSCRecHit
DataFormats/TrackReco
RecoParticleFlow/PFSimProducer
RecoParticleFlow/PFTracking

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @battibass, @makortel, @folguera, @abbiendi, @drkovalskyi, @rovere, @lgray, @calderona, @ptcox, @HuguesBrun, @seemasharmafnal, @cbernet, @gpetruc, @VinInn, @trocino, @bachtis, @jhgoh this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

perrotta · 2019-01-07T12:45:33Z

please test

cmsbuild · 2019-01-07T12:46:01Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32428/console Started: 2019/01/07 13:47

perrotta

After a quick code inspection, I found a couple of indentations to be fixed

DataFormats/CSCRecHit/test/CSCSharesInputTest.cc

RecoParticleFlow/PFTracking/plugins/PFConversionProducer.cc

cmsbuild · 2019-01-07T17:06:15Z

+1
Tested at: f4d5615
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25591/32428/summary.html

cmsbuild · 2019-01-07T17:06:19Z

Comparison job queued.

makortel · 2019-01-07T17:32:25Z

DataFormats/TrackReco/interface/TrackExtraBase.h

@@ -25,6 +25,18 @@ class TrackExtraBase
    using TrajParams = std::vector<LocalTrajectoryParameters>;
    using Chi2sFive = std::vector<unsigned char>;

+    /// class which implemebts begin() and end() to enable range-based loop on RecHits
+    class TrackingRecHitCollectionWindow {


I think TrackingRecHitRange would be a better name (closer to standard naming convention). In which case it would probably be better to be defined in DataFormats/TrackingRecHit. (we could even consider a new class template that could then be instantiated in DataFormats/TrackingRechit/interface/TrackingRecHitFwd.h)

Indeed, that name is shorter and better. So I understand correctly that you'd define the class in DataFormats/TrackingRechit/interface/TrackingRecHitFwd.h? But where would you put the template? Actually I don't know if that pattern reoccurs in CMSSW and the template is necessary. Thanks for your advice!

So I understand correctly that you'd define the class in DataFormats/TrackingRechit/interface/TrackingRecHitFwd.h

That file is for forward declarations so I wouldn't define the TrackingRecHitRange class there (unless it would be a template instantiation).

But where would you put the template?

I'm thinking edm::ConstRange<T> (with T being the iterator type) in either FWCore/Utilities or DataFormats/Common. @Dr15Jones, would you have any input?

Actually I don't know if that pattern reoccurs in CMSSW and the template in necessary.

I don't recall seeing this pattern, but many times in the past I would have wanted to use range-for for iterator pairs obtained with something else that begin()+end() pair (which reminds me that I may have introduced some ad-hoc range classes somewhere, but can't remember where). I just believe this pattern would have potential to be used elsewhere as well.

Ok! So that means if it wouldn't be a template it belongs in DataFormats/TrackingRechit/interface/TrackingRecHit.h without the Fwd? Hope I understand this well. Alright, let's see what Chris says.

I just recalled there would maybe be more use-cases for such a template. There is for example not only the *Begin()/*End() pattern that was seen in the tracks, but sometimes people just created pairs to imitate a range, like here:

https://github.com/cms-sw/cmssw/pull/25591/files#diff-2718c8ec8e06c5b5679d465a9c81b6e8R426

If I'm not mistaken I have seen such things elsewhere.

Since the location of the file is not so important after all, I just suggested how the edm::ConstRange<T> could look like.

So that means if it wouldn't be a template it belongs in DataFormats/TrackingRechit/interface/TrackingRecHit.h without the Fwd?

Or in a separate header.

I just recalled there would maybe be more use-cases for such a template. There is for example not only the *Begin()/*End() pattern that was seen in the tracks, but sometimes people just created pairs to imitate a range, like here:

Yeah, there are plenty of those. Many (if not all) of those predate C++11 and range-for.

cmsbuild · 2019-01-07T18:40:21Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25591/32428/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3153717
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3153512
DQMHistoTests: Total skipped: 204
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

cmsbuild · 2019-01-10T16:22:12Z

Pull request #25591 was updated. @perrotta, @smuzaffar, @cmsbuild, @Dr15Jones, @slava77 can you please check and sign again.

Dr15Jones · 2019-01-10T16:41:14Z

+1
for FWCore/Utilities change.

cmsbuild · 2019-01-10T16:41:42Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32519/console Started: 2019/01/10 17:42

cmsbuild · 2019-01-10T21:31:24Z

+1
Tested at: 8b60637
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25591/32519/summary.html

cmsbuild · 2019-01-10T21:31:28Z

Comparison job queued.

cmsbuild · 2019-01-10T23:10:07Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25591/32519/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3153717
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3153511
DQMHistoTests: Total skipped: 204
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

slava77 · 2019-01-13T04:47:07Z

+1

for #25591 8b60637

code changes are in line with the PR description and the follow up review by @makortel (Thanks!). This is a technical change.
jenkins tests pass and comparisons with the baseline show no (relevant) differences

cmsbuild · 2019-01-13T04:47:32Z

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-01-14T21:45:36Z

+1

New TrackingRecHitCollectionWindow and use it in RecoParticleFlow

f4d5615

cmsbuild added this to the CMSSW_10_5_X milestone Jan 7, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jan 7, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 7, 2019

cmsbuild added tests-started and removed tests-pending labels Jan 7, 2019

perrotta reviewed Jan 7, 2019

View reviewed changes

DataFormats/CSCRecHit/test/CSCSharesInputTest.cc Outdated Show resolved Hide resolved

RecoParticleFlow/PFTracking/plugins/PFConversionProducer.cc Outdated Show resolved Hide resolved

cmsbuild added tests-approved and removed tests-started labels Jan 7, 2019

makortel reviewed Jan 7, 2019

View reviewed changes

cmsbuild added comparison-available code-checks-pending and removed comparison-pending code-checks-approved comparison-available tests-approved labels Jan 7, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 10, 2019

cmsbuild added core-approved and removed core-pending labels Jan 10, 2019

cmsbuild added tests-started and removed tests-pending labels Jan 10, 2019

cmsbuild added tests-approved and removed tests-started labels Jan 10, 2019

cmsbuild added comparison-available and removed comparison-pending labels Jan 10, 2019

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jan 13, 2019

cmsbuild added orp-approved and removed orp-pending labels Jan 14, 2019

cmsbuild merged commit d9f3569 into cms-sw:master Jan 14, 2019

guitargeek mentioned this pull request Jan 14, 2019

Some more use of new TrackingRecHitRange #25663

Merged

guitargeek deleted the track_recHits branch January 14, 2019 22:06

perrotta mentioned this pull request Jan 17, 2019

Fix compilation warning #25687

Merged

guitargeek mentioned this pull request May 25, 2020

Better TrajectorySeed interface to recHits #29971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New TrackingRecHitRange and use it in RecoParticleFlow #25591

New TrackingRecHitRange and use it in RecoParticleFlow #25591

guitargeek commented Jan 7, 2019 •

edited

cmsbuild commented Jan 7, 2019

cmsbuild commented Jan 7, 2019

cmsbuild commented Jan 7, 2019

perrotta commented Jan 7, 2019

cmsbuild commented Jan 7, 2019 •

edited

perrotta left a comment

cmsbuild commented Jan 7, 2019

cmsbuild commented Jan 7, 2019

makortel Jan 7, 2019

guitargeek Jan 7, 2019 •

edited

makortel Jan 7, 2019

guitargeek Jan 7, 2019 •

edited

guitargeek Jan 7, 2019

makortel Jan 7, 2019

cmsbuild commented Jan 7, 2019

cmsbuild commented Jan 10, 2019

Dr15Jones commented Jan 10, 2019

cmsbuild commented Jan 10, 2019 •

edited

cmsbuild commented Jan 10, 2019

cmsbuild commented Jan 10, 2019

cmsbuild commented Jan 10, 2019

slava77 commented Jan 13, 2019

cmsbuild commented Jan 13, 2019

fabiocos commented Jan 14, 2019

New TrackingRecHitRange and use it in RecoParticleFlow #25591

New TrackingRecHitRange and use it in RecoParticleFlow #25591

Conversation

guitargeek commented Jan 7, 2019 • edited

cmsbuild commented Jan 7, 2019

cmsbuild commented Jan 7, 2019

cmsbuild commented Jan 7, 2019

perrotta commented Jan 7, 2019

cmsbuild commented Jan 7, 2019 • edited

perrotta left a comment

Choose a reason for hiding this comment

cmsbuild commented Jan 7, 2019

cmsbuild commented Jan 7, 2019

makortel Jan 7, 2019

Choose a reason for hiding this comment

guitargeek Jan 7, 2019 • edited

Choose a reason for hiding this comment

makortel Jan 7, 2019

Choose a reason for hiding this comment

guitargeek Jan 7, 2019 • edited

Choose a reason for hiding this comment

guitargeek Jan 7, 2019

Choose a reason for hiding this comment

makortel Jan 7, 2019

Choose a reason for hiding this comment

cmsbuild commented Jan 7, 2019

cmsbuild commented Jan 10, 2019

Dr15Jones commented Jan 10, 2019

cmsbuild commented Jan 10, 2019 • edited

cmsbuild commented Jan 10, 2019

cmsbuild commented Jan 10, 2019

cmsbuild commented Jan 10, 2019

slava77 commented Jan 13, 2019

cmsbuild commented Jan 13, 2019

fabiocos commented Jan 14, 2019

guitargeek commented Jan 7, 2019 •

edited

cmsbuild commented Jan 7, 2019 •

edited

guitargeek Jan 7, 2019 •

edited

guitargeek Jan 7, 2019 •

edited

cmsbuild commented Jan 10, 2019 •

edited