Cleanup of Cellular Automaton seeding code #20177

makortel · 2017-08-15T14:51:54Z

This PR cleans up unused pieces of the CA hit triplet/quadruplet finding code (the duplication of code between triplet and quadruplet is still left there as we'll try to unify them to a more generic ntuplet finder). Essentially this means the support towards the "old seeding framework", simplifying the code to serve a clearer basis for further development.

Tested in 9_3_0_pre3, no changes expected.

@rovere @VinInn @felicepantaleo @JanFSchulte @ebrondol

This means also removal of support for old-style seeding framework, but I'm not sure that interface would have worked anyway nowadays because of earlier changes.

cmsbuild · 2017-08-15T14:52:12Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

RecoPixelVertexing/PixelTriplets

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @ebrondol, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

makortel · 2017-08-15T14:53:33Z

@cmsbuild, please test

cmsbuild · 2017-08-15T14:53:51Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22317/console Started: 2017/08/15 16:54

cmsbuild · 2017-08-15T14:54:16Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20177/133

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20177/133/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20177/133/git-diff.patch | patch -p1

In future, you can run scram build code-checks to apply code checks

makortel · 2017-08-15T15:00:05Z

What is the logic to select the files to be given to the code style checker?

cmsbuild · 2017-08-15T16:25:44Z

+1
Tested at: 8eb42df
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20177/22317/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
7d6f190
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20177/22317/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20177/22317/git-merge-result

cmsbuild · 2017-08-15T16:25:47Z

Comparison job queued.

rovere · 2017-08-15T16:33:10Z

Maybe the files modified by the PR?

On Tue, Aug 15, 2017, 17:01 Matti Kortelainen ***@***.***> wrote: What is the logic to select the files to be given to the code style checker? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20177 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABHaR9uixniQJx1yKgZE6PVBMZxUhFHRks5sYbJ5gaJpZM4O3r_6> .

-- Ciao, --Marco.

…

___________ Marco Rovere Marco.Rovere@cern.ch CERN EP-CMG-CO | room 40 3-A28 | tel +41227671209 (71209)

cmsbuild · 2017-08-15T19:15:07Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2718193
DQMHistoTests: Total failures: 286
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2717718
DQMHistoTests: Total skipped: 189
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 14 edm output root files, 26 DQM output files

makortel · 2017-08-16T08:39:53Z

Maybe the files modified by the PR?

I think I got the logic now. I got confused because the suggested patch
https://cmssdt.cern.ch/SDT/code-checks/PR-20177/133/git-diff.patch
touches mostly files I did not edit (except CAHitNtupletEDProducerT.cc), but of course if I touch a .cc file, the checker also checks all the included headers.

What I don't understand though is how PixelQuadrupletMergerEDProducer.cc and QuadrupletSeedMerger.cc ended up in the list of selected files
https://cmssdt.cern.ch/SDT/code-checks/PR-20177/133/changed-files-selected.log
(or that's what I assume that this is the list of files to be checked by clang-tidy).

slava77 · 2017-08-21T14:24:49Z

RecoPixelVertexing/PixelTriplets/plugins/CAHitQuadrupletGenerator.h

-    CAHitQuadrupletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector&& iC, bool needSeedingLayerSetsHits=true): CAHitQuadrupletGenerator(cfg, iC, needSeedingLayerSetsHits) {}
-    CAHitQuadrupletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector& iC, bool needSeedingLayerSetsHits=true);
+    CAHitQuadrupletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector&& iC): CAHitQuadrupletGenerator(cfg, iC) {}
+    CAHitQuadrupletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector& iC);

    virtual ~CAHitQuadrupletGenerator();


is virtual still needed here?

I suppose is in https://cmssdt.cern.ch/SDT/code-checks/PR-20177/133/git-diff.patch
that in my understanding has not been applied yet...

To be more clear: CAHitQuadrupletGenerator does not derive from anything anymore and there is no derived class from it either. Why do we need a virtual destructor in this case?

slava77 · 2017-08-21T14:27:18Z

RecoPixelVertexing/PixelTriplets/plugins/CAHitTripletGenerator.h

-    CAHitTripletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector&& iC, bool needSeedingLayerSetsHits=true): CAHitTripletGenerator(cfg, iC, needSeedingLayerSetsHits) {}
-    CAHitTripletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector& iC, bool needSeedingLayerSetsHits=true);
+    CAHitTripletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector&& iC): CAHitTripletGenerator(cfg, iC) {}
+    CAHitTripletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector& iC);

    virtual ~CAHitTripletGenerator();


is virtual still needed?

makortel · 2017-08-21T19:02:19Z

Thanks Slava, the virtual destructor is certainly not needed (slipped through my eyes). I'll remove it early next weeknor so.

…

On 21 August 2017 16:42:54 CEST, Slava Krutelyov ***@***.***> wrote: slava77 commented on this pull request. > @@ -38,8 +37,8 @@ class CAHitQuadrupletGenerator : public HitQuadrupletGenerator { public: - CAHitQuadrupletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector&& iC, bool needSeedingLayerSetsHits=true): CAHitQuadrupletGenerator(cfg, iC, needSeedingLayerSetsHits) {} - CAHitQuadrupletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector& iC, bool needSeedingLayerSetsHits=true); + CAHitQuadrupletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector&& iC): CAHitQuadrupletGenerator(cfg, iC) {} + CAHitQuadrupletGenerator(const edm::ParameterSet& cfg, edm::ConsumesCollector& iC); virtual ~CAHitQuadrupletGenerator(); To be more clear: CAHitQuadrupletGenerator does not derive from anything anymore and there is no derived class from it either. Why do we need a virtual destructor in this case?

felicepantaleo · 2017-08-23T19:11:02Z

RecoPixelVertexing/PixelTriplets/plugins/CAHitQuadrupletGenerator.cc

@@ -54,18 +54,11 @@ caPhiCut(cfg.getParameter<double>("CAPhiCut")),
 caHardPtCut(cfg.getParameter<double>("CAHardPtCut")),
 caOnlyOneLastHitPerLayerFilter(cfg.getParameter<bool>("CAOnlyOneLastHitPerLayerFilter"))


hi @makortel , could you also get rid of the caOnlyOneLastHitPerLayerFilter variable in this PR?

cmsbuild · 2017-08-28T10:55:40Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-08-28T10:55:41Z

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

makortel · 2017-08-28T10:57:04Z

@cmsbuild, please test

removed virtual from CAHit*Generator destructors (suggested by @slava77)
removed CAOnlyOneLastHitPerLayerFilter parameter (suggested by @felicepantaleo)
applied code check patch
removed obsolete python configuration files

cmsbuild · 2017-08-28T10:57:23Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22518/console Started: 2017/08/28 12:57

cmsbuild · 2017-08-28T10:57:29Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20177/341

cmsbuild · 2017-08-28T12:02:12Z

+1
Tested at: 2be23e0
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20177/22518/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
5ca2c40
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20177/22518/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20177/22518/git-merge-result

cmsbuild · 2017-08-28T12:02:15Z

Comparison job queued.

cmsbuild · 2017-08-28T12:47:38Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2653934
DQMHistoTests: Total failures: 203
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2653542
DQMHistoTests: Total skipped: 189
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 14 edm output root files, 26 DQM output files

slava77 · 2017-08-30T22:32:46Z

+1

for #20177 2be23e0

cleanup of obsolete CA code; clang tidy
jenkins tests pass and comparisons show no differences

cmsbuild · 2017-08-30T22:33:07Z

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 (and backports should be raised in the release meeting by the corresponding L2)

davidlange6 · 2017-08-31T07:34:07Z

+1

makortel added 2 commits August 11, 2017 11:51

Remove unused pieces of CAHit{Quadruplet,Triplet}Generator

39caddb

This means also removal of support for old-style seeding framework, but I'm not sure that interface would have worked anyway nowadays because of earlier changes.

Add missing header

8eb42df

cmsbuild added this to the CMSSW_9_3_X milestone Aug 15, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 15, 2017

cmsbuild added tests-started and removed tests-pending labels Aug 15, 2017

cmsbuild added tests-approved and removed tests-started labels Aug 15, 2017

cmsbuild added comparison-available and removed comparison-pending labels Aug 15, 2017

slava77 reviewed Aug 21, 2017

View reviewed changes

smuzaffar added the code-checks-rejected label Aug 22, 2017

felicepantaleo reviewed Aug 23, 2017

View reviewed changes

Remove virtual destructors as they became unnecessary

38c4329

cmsbuild added code-checks-pending comparison-pending tests-pending labels Aug 28, 2017

cmsbuild added code-checks-approved tests-started and removed code-checks-pending tests-pending labels Aug 28, 2017

cmsbuild added tests-approved and removed tests-started labels Aug 28, 2017

cmsbuild added comparison-available and removed comparison-pending labels Aug 28, 2017

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Aug 30, 2017

cmsbuild added orp-approved and removed orp-pending labels Aug 31, 2017

cmsbuild merged commit 94fab3e into cms-sw:master Aug 31, 2017

makortel deleted the cleanupCA branch February 12, 2018 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup of Cellular Automaton seeding code #20177

Cleanup of Cellular Automaton seeding code #20177

makortel commented Aug 15, 2017

cmsbuild commented Aug 15, 2017

makortel commented Aug 15, 2017

cmsbuild commented Aug 15, 2017 •

edited

cmsbuild commented Aug 15, 2017

makortel commented Aug 15, 2017

cmsbuild commented Aug 15, 2017

cmsbuild commented Aug 15, 2017

rovere commented Aug 15, 2017 via email

cmsbuild commented Aug 15, 2017

makortel commented Aug 16, 2017

slava77 Aug 21, 2017

VinInn Aug 21, 2017

slava77 Aug 21, 2017

slava77 Aug 21, 2017

makortel commented Aug 21, 2017 via email

felicepantaleo Aug 23, 2017

cmsbuild commented Aug 28, 2017

cmsbuild commented Aug 28, 2017

makortel commented Aug 28, 2017

cmsbuild commented Aug 28, 2017 •

edited

cmsbuild commented Aug 28, 2017

cmsbuild commented Aug 28, 2017

cmsbuild commented Aug 28, 2017

cmsbuild commented Aug 28, 2017

slava77 commented Aug 30, 2017

cmsbuild commented Aug 30, 2017

davidlange6 commented Aug 31, 2017

		@@ -54,18 +54,11 @@ caPhiCut(cfg.getParameter<double>("CAPhiCut")),
		caHardPtCut(cfg.getParameter<double>("CAHardPtCut")),
		caOnlyOneLastHitPerLayerFilter(cfg.getParameter<bool>("CAOnlyOneLastHitPerLayerFilter"))

Cleanup of Cellular Automaton seeding code #20177

Cleanup of Cellular Automaton seeding code #20177

Conversation

makortel commented Aug 15, 2017

cmsbuild commented Aug 15, 2017

makortel commented Aug 15, 2017

cmsbuild commented Aug 15, 2017 • edited

cmsbuild commented Aug 15, 2017

makortel commented Aug 15, 2017

cmsbuild commented Aug 15, 2017

cmsbuild commented Aug 15, 2017

rovere commented Aug 15, 2017 via email

cmsbuild commented Aug 15, 2017

makortel commented Aug 16, 2017

slava77 Aug 21, 2017

Choose a reason for hiding this comment

VinInn Aug 21, 2017

Choose a reason for hiding this comment

slava77 Aug 21, 2017

Choose a reason for hiding this comment

slava77 Aug 21, 2017

Choose a reason for hiding this comment

makortel commented Aug 21, 2017 via email

felicepantaleo Aug 23, 2017

Choose a reason for hiding this comment

cmsbuild commented Aug 28, 2017

cmsbuild commented Aug 28, 2017

makortel commented Aug 28, 2017

cmsbuild commented Aug 28, 2017 • edited

cmsbuild commented Aug 28, 2017

cmsbuild commented Aug 28, 2017

cmsbuild commented Aug 28, 2017

cmsbuild commented Aug 28, 2017

slava77 commented Aug 30, 2017

cmsbuild commented Aug 30, 2017

davidlange6 commented Aug 31, 2017

cmsbuild commented Aug 15, 2017 •

edited

cmsbuild commented Aug 28, 2017 •

edited