Modernize code and simplify GeometricSearchTrackerBuilder #33420

perrotta · 2021-04-13T11:11:18Z

PR description:

The possible simplifications and cleanings that were proposed during the review of PR 33272 are tentatively implemented here:

Remove unneeded includes of "Geometry/TrackerGeometryBuilder/interface/trackerHierarchy.h" and "DataFormats/Common/interface/Trie.h"
Range based loops for the current it1 and all it2 iterators would simplify the code and improve its readability
An else if can be used in all checks for positionBounds()->z(), to avoid repeating those checks when the first one is passed
Avoid redefining useBrothers at L172, as it was already identically defined at L46

No changes are expected in output

@parbol @fabiocos
@mtosi @JanFSchulte @vmariani @ebrondol

PR validation:

Short matrix runs without errors

cmsbuild · 2021-04-13T11:17:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33420/22072

This PR adds an extra 16KB to repository

cmsbuild · 2021-04-13T11:17:37Z

A new Pull Request was created by @perrotta for master.

It involves the following packages:

RecoTracker/TkDetLayers

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @gpetruc, @ebrondol, @mtosi, @dgulhan this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

perrotta · 2021-04-13T12:01:37Z

please test

VinInn · 2021-04-13T12:11:10Z

RecoTracker/TkDetLayers/src/GeometricSearchTrackerBuilder.cc

-           it2++) {
-        thePxlBarLayers.push_back(aPixelBarrelLayerBuilder.build(*it2, theGeomDetGeometry));
+    if (theGeomDetLayer->type() == GeometricDet::PixelPhase1Barrel) {
+      vector<const GeometricDet *> thePxlBarGeometricDetLayers = theGeomDetLayer->components();


auto const & ?

I'd personally prefer to keep the original explicit declaration as it was before (here and in the other similar cases that you pointed out before): do you have any issue with it? You are the code expert, and I'll follow your prescription in that case

try to add const & anyhow. Should help the compiler not to copy the vector!

Btw modenrize (in my opinion) is also use auto when is not confusing

VinInn · 2021-04-13T12:12:09Z

RecoTracker/TkDetLayers/src/GeometricSearchTrackerBuilder.cc

-        thePxlBarLayers.push_back(aPixelBarrelLayerBuilder.build(*it2, theGeomDetGeometry));
+    if (theGeomDetLayer->type() == GeometricDet::PixelPhase1Barrel) {
+      vector<const GeometricDet *> thePxlBarGeometricDetLayers = theGeomDetLayer->components();
+      for (const GeometricDet *thisGeomDet : thePxlBarGeometricDetLayers) {


auto const ?

VinInn · 2021-04-13T12:15:22Z

RecoTracker/TkDetLayers/src/GeometricSearchTrackerBuilder.cc

+      for (const GeometricDet *thisGeomDet : thePxlFwdGeometricDetLayers) {
+        if ((thisGeomDet)->positionBounds().z() < 0)
+          theNegPxlFwdLayers.push_back(aPhase1PixelForwardLayerBuilder.build(thisGeomDet, theGeomDetGeometry));
+        else if ((thisGeomDet)->positionBounds().z() > 0)


else without the test for z>0?

else without the test for z>0?

Thank you for your comments, Vincenzo
I don't understand this one though...

it can be either z<0 or z>0 (z==0 is not possible)
so if is not z<0 MUST BE z>0 no need to test

there are many other occurrencies below

there are many other occurrencies below

Thank you @VinInn

I think I fixed all them, but this one:
https://github.com/cms-sw/cmssw/pull/33420/files#diff-0b80efba96a5198928106c0248e4976130ec9316fde648c6738d77951bf2f0b3R119
because it explicitly admitted a further else statement already in the original code, which triggers a LogError in its turn

cmsbuild · 2021-04-13T14:45:12Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-85f43d/14211/summary.html
COMMIT: a745b40
CMSSW: CMSSW_11_3_X_2021-04-12-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33420/14211/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 38
DQMHistoTests: Total histograms compared: 2865526
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2865503
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
Checked 160 log files, 37 edm output root files, 38 DQM output files
TriggerResults: no differences found

cmsbuild · 2021-04-13T14:58:58Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33420/22085

This PR adds an extra 16KB to repository

cmsbuild · 2021-04-13T14:59:21Z

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

cmsbuild · 2021-04-15T10:33:25Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33420/22120

This PR adds an extra 16KB to repository

cmsbuild · 2021-04-15T10:33:50Z

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

perrotta · 2021-04-15T10:35:38Z

There are unexpected differences in the conversions in wf 28234.0: to be understood...

The differences visible in the tests are only in the converted photons, and apparently not in any other tracker/tracking related quantity.
There is one less conv photon at z=0: I have tried to check whether there was some special (pathological?) events in 28234.0 in which considering also the possible case thisGeomDet->positionBounds().z()=0 would have added something, but I found no cases in the events at hand, as it should.
Let retry running the tests here once again, and check whether the difference in the converted photons is reproducible.

perrotta · 2021-04-15T10:35:44Z

please test

cmsbuild · 2021-04-15T13:09:58Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-85f43d/14254/summary.html
COMMIT: ddcfd6e
CMSSW: CMSSW_11_3_X_2021-04-14-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33420/14254/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 38
DQMHistoTests: Total histograms compared: 2864426
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2864397
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 37 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 160 log files, 37 edm output root files, 38 DQM output files
TriggerResults: no differences found

perrotta · 2021-04-15T13:14:08Z

Differences in the converted photons have disappeared...

perrotta · 2021-04-19T06:48:13Z

+1

Technical, just some adjustment to the c++ code, with no effect expected on the outputs
Jenkins tests pass

cmsbuild · 2021-04-19T06:48:36Z

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

qliphy · 2021-04-19T07:47:16Z

+1

cmsbuild added this to the CMSSW_11_3_X milestone Apr 13, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 13, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 13, 2021

cmsbuild added tests-started and removed tests-pending labels Apr 13, 2021

VinInn reviewed Apr 13, 2021

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Apr 13, 2021

Modernize code and simplify

1ff8e1f

perrotta force-pushed the simplifyGeometricSearchTrackerBuilder branch from a745b40 to 1ff8e1f Compare April 13, 2021 14:52

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Apr 13, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 13, 2021

Use auto

932f6ae

use references

ddcfd6e

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Apr 15, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 15, 2021

cmsbuild added tests-started and removed tests-pending labels Apr 15, 2021

cmsbuild added tests-approved and removed tests-started labels Apr 15, 2021

cmsbuild modified the milestones: CMSSW_11_3_X, CMSSW_12_0_X Apr 15, 2021

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Apr 19, 2021

cmsbuild added orp-approved and removed orp-pending labels Apr 19, 2021

cmsbuild merged commit 3bb4989 into cms-sw:master Apr 19, 2021

perrotta deleted the simplifyGeometricSearchTrackerBuilder branch April 19, 2021 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize code and simplify GeometricSearchTrackerBuilder #33420

Modernize code and simplify GeometricSearchTrackerBuilder #33420

perrotta commented Apr 13, 2021

cmsbuild commented Apr 13, 2021

cmsbuild commented Apr 13, 2021

perrotta commented Apr 13, 2021

VinInn Apr 13, 2021

perrotta Apr 13, 2021

VinInn Apr 13, 2021

VinInn Apr 13, 2021

VinInn Apr 13, 2021

perrotta Apr 13, 2021

VinInn Apr 13, 2021

VinInn Apr 13, 2021 •

edited

Loading

perrotta Apr 13, 2021

cmsbuild commented Apr 13, 2021

cmsbuild commented Apr 13, 2021

cmsbuild commented Apr 13, 2021

cmsbuild commented Apr 15, 2021

cmsbuild commented Apr 15, 2021

perrotta commented Apr 15, 2021

perrotta commented Apr 15, 2021

cmsbuild commented Apr 15, 2021

perrotta commented Apr 15, 2021

perrotta commented Apr 19, 2021

cmsbuild commented Apr 19, 2021

qliphy commented Apr 19, 2021

Modernize code and simplify GeometricSearchTrackerBuilder #33420

Modernize code and simplify GeometricSearchTrackerBuilder #33420

Conversation

perrotta commented Apr 13, 2021

PR description:

PR validation:

cmsbuild commented Apr 13, 2021

cmsbuild commented Apr 13, 2021

perrotta commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Apr 13, 2021

Comparison Summary

cmsbuild commented Apr 13, 2021

cmsbuild commented Apr 13, 2021

cmsbuild commented Apr 15, 2021

cmsbuild commented Apr 15, 2021

perrotta commented Apr 15, 2021

perrotta commented Apr 15, 2021

cmsbuild commented Apr 15, 2021

Comparison Summary

perrotta commented Apr 15, 2021

perrotta commented Apr 19, 2021

cmsbuild commented Apr 19, 2021

qliphy commented Apr 19, 2021

VinInn Apr 13, 2021 •

edited

Loading