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.

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

Quadruplet seeding by propagating triplet to 4th layer #13753

Merged
merged 5 commits into from Mar 29, 2016

Conversation

makortel
Copy link
Contributor

This PR adds code for the first prototype of quadruplet seeding by "propagating triplet to 4th layer and searching for compatible hits", discussed in TRK POG meetings March 14 https://indico.cern.ch/event/508493/contribution/3/attachments/1242946/1828945/slides.pdf and Feb 23 2015 https://indico.cern.ch/event/361404/contribution/3/attachments/719405/987513/slides.pdf. The algorithm is disabled by default, but a customize --customise RecoTracker/Configuration/customiseForQuadrupletsByPropagation.customiseForQuadrupletsByPropagation switching all triplet-merging cases to propagation in PixelTrackProducer and in seeding.

Although being still work in progress and disabled by default, integrating the code eases its maintenance and testing by other people.

Tested in 8_0_1_pre1, no changes expected in standard workflows.

I have here two sets of MTV plots (with 1000 TTbar+PU events in CMSSW_8_1_X_2016-03-15-1100)

@rovere @VinInn

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Calibration/IsolatedParticles
HLTrigger/Configuration
RecoHI/HiTracking
RecoMuon/L3MuonProducer
RecoPixelVertexing/PixelTrackFitting
RecoPixelVertexing/PixelTriplets
RecoTracker/Configuration

@perrotta, @cmsbuild, @diguida, @cvuosalo, @fwyzard, @cerminar, @Martin-Grunewald, @franzoni, @slava77, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @abbiendi, @echapon, @geoff-smith, @battibass, @jhgoh, @dgulhan, @cerati, @trocino, @yetkinyilmaz, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @tocheng, @jalimena, @mschrode, @richard-cms, @Martin-Grunewald, @mandrenguyen, @jazzitup, @yenjie, @kurtejung, @gpetruc, @istaslis, @rociovilar, @bachtis this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@makortel makortel changed the title First prototype of quadruplet seeding algorithm for 2017 Quadruplet seeding by propagating triplet to 4th layer Mar 17, 2016
@VinInn
Copy link
Contributor

VinInn commented Mar 17, 2016

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11942/console

@cmsbuild
Copy link
Contributor

-1
Tested at: 244c235
I found a compilation error while trying to compile with clang:
I used this command:
scram b vclean && scram build -k -j 16 USER_CXXFLAGS='-fsyntax-only' COMPILER='llvm compile'

               ^
/cvmfs/cms-ib.cern.ch/week0/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_8_1_X_2016-03-16-2300/src/FWCore/Framework/interface/one/EDAnalyzerBase.h:98:20: note: overridden virtual function is here
      virtual void beginJob() {}
                   ^
>> Compiling edm plugin /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-03-16-2300/src/RecoPixelVertexing/PixelTriplets/plugins/SealModule.cc 
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-03-16-2300/src/RecoPixelVertexing/PixelTriplets/plugins/PixelQuadrupletGenerator.cc:66:46: error: variable length array of non-POD element type 'ThirdHitRZPrediction'
  ThirdHitRZPrediction preds[size];
                                             ^
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-03-16-2300/src/RecoPixelVertexing/PixelTriplets/plugins/PixelQuadrupletGenerator.cc:112:21: error: constexpr variable 'nSigmaRZ' must be initialized by a constant expression
    constexpr float nSigmaRZ = std::sqrt(12.f);
                    ^          ~~~~~~~~~~~~~~~


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13753/11942/summary.html

@cmsbuild
Copy link
Contributor

Pull request #13753 was updated. @perrotta, @cmsbuild, @diguida, @cvuosalo, @fwyzard, @cerminar, @Martin-Grunewald, @franzoni, @slava77, @mmusich, @davidlange6 can you please check and sign again.

@makortel
Copy link
Contributor Author

Some modernizations were missing, should be fixed now.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Mar 17, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11946/console

@cvuosalo
Copy link
Contributor

An extended test of workflow 10224.0_TTbar_13 (which is a 2017 workflow) with the algorithm enabled (as explained in the description) and with 70 events against baseline CMSSW_8_1_X_2016-03-10-2300 with the algorithm disabled shows no significant increase in time or memory usage. Note these tests included both the DQM and Validation steps.
Summary of time and memory:

Baseline
Max VSIZ 2835.09 on evt 67 ; max RSS 2189.01 on evt 68
Time av 100.102 s/evt   max 148.093 s on evt 1
M1 Time av 99.4069 s/evt   max 124.892 s on evt 20
M8 Time av 99.3914 s/evt   max 124.892 s on evt 20

PR 13753
Max VSIZ 2821.64 on evt 66 ; max RSS 2202.91 on evt 68
Time av 99.7959 s/evt   max 148.632 s on evt 1
M1 Time av 99.0882 s/evt   max 124.447 s on evt 20
M8 Time av 98.8341 s/evt   max 124.447 s on evt 20

The time for various tracking steps increased or decreased, but the overall time showed a slight decrease (times exclude first event):

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
   -1.035849      -0.80%      1145.45 ms/ev ->       363.78 ms/ev lowPtQuadStepTrackCandidates
   -0.970964      -0.11%       164.94 ms/ev ->        57.13 ms/ev lowPtQuadStepTracks
   -0.764124      -0.12%       217.07 ms/ev ->        97.05 ms/ev initialStepSeeds
   -0.755311      -0.34%       601.11 ms/ev ->       271.55 ms/ev lowPtQuadStepSeeds
   -0.680000      -0.21%       410.38 ms/ev ->       202.13 ms/ev detachedQuadStepSeeds
   +0.281839      +0.36%      1061.51 ms/ev ->      1409.76 ms/ev lowPtTripletStepTrackCandidates

Another measurement gives a similar picture:

    initialStepSeeds     154.156 ms/ev -> 68.7239 ms/ev
    initialStepTrackCandidates   523.191 ms/ev -> 482.233 ms/ev
    initialStepTracks    119.491 ms/ev -> 99.0642 ms/ev
    highPtTripletStepTrackCandidates     539.739 ms/ev -> 576.283 ms/ev
    lowPtQuadStepSeeds   425.07 ms/ev -> 191.984 ms/ev
    lowPtQuadStepTrackCandidates     812.064 ms/ev -> 258.176 ms/ev
    lowPtQuadStepTracks      117.154 ms/ev -> 40.6786 ms/ev
    lowPtTripletStepSeeds    238.74 ms/ev -> 285.617 ms/ev
    lowPtTripletStepTrackCandidates      752.196 ms/ev -> 1003.15 ms/ev
    lowPtTripletStepTracks   94.8293 ms/ev -> 116.024 ms/ev
    detachedQuadStepSeeds    290.684 ms/ev -> 143.29 ms/ev
    detachedQuadStepTrackCandidates      86.5754 ms/ev -> 44.9036 ms/ev
    detachedQuadStepTracks   50.5428 ms/ev -> 26.9334 ms/ev
    mixedTripletStepSeedsA   234.518 ms/ev -> 255.705 ms/ev
    mixedTripletStepSeedsB   116.791 ms/ev -> 142.697 ms/ev

@cvuosalo
Copy link
Contributor

The same extended test described above produces extensive differences in tracking quantities, as might be expected. However, most all of the changes preserve the shapes of distributions even while possibly changing the number of histogram entries. But at least two DQM plots show significant shape changes:

eta
eta2

@cvuosalo
Copy link
Contributor

The same extended test described above shows a 2% decrease in the size of RECO event content. The largest changes are:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
   1641.9 ->      1484.4       -157    -10.1  -0.01     recoJetedmRefToBaseProdrecoTracksrecoTrackrecoTracksTorecoTrackedmrefhelperFindUsingAdvanceedmRefVectorsAssociationVector_ak4JetTracksAssociatorAtVertexPF__RECO.
   5618.4 ->      5055.6       -563    -10.5  -0.03     TrackingParticlesrecoTrackedmViewdoubleuintTrackingParticlesedmRefProdrecoTrackedmRefToBaseProdTrackingParticlesTrackingParticleTrackingParticlesTrackingParticleedmrefhelperFindUsingAdvanceedmRefrecoTrackedmRefToBaseedmOneToManyWithQualityGenericedmAssociationMap_trackingParticleRecoTrackAsssociation__RECO.
  98606.3 ->     88713.7      -9893    -10.6  -0.46     recoTracks_generalTracks__RECO.
   5723.4 ->      5178.5       -545    -10.0  -0.03     recoTrackedmViewTrackingParticlesdoubleuintrecoTrackedmRefToBaseProdTrackingParticlesedmRefProdrecoTrackedmRefToBaseTrackingParticlesTrackingParticleTrackingParticlesTrackingParticleedmrefhelperFindUsingAdvanceedmRefedmOneToManyWithQualityGenericedmAssociationMap_trackingParticleRecoTrackAsssociation__RECO.
 175488.0 ->    157087.0     -18401    -11.1  -0.86     recoTrackExtras_generalTracks__RECO.

@slava77
Copy link
Contributor

slava77 commented Mar 22, 2016

@cvuosalo what are the totals of time changes in the reco steps/tracking steps?
From the middle printout it looks like a net decrease of 1.3s (40% net reduction in the printed set; it's probably about 10% of full reco step, unless some other module takes over) #13753 (comment)

10% is a large difference (maybe that fits the "slight decrease" in your summary).

Is the DQM comparison appropriately normalized? There is a ~40% discrepancy.

@cvuosalo
Copy link
Contributor

@slava77:
The wall-clock time difference is:

Job total:  97.3583 s/ev ==> 96.9674 s/ev (-0.416 s/ev, -0.4%)

The module time difference is:

Total in detailed printout:     5951.88 ms/ev -> 5041.42 ms/ev          delta: -910.462 (-15%)

Note that the DQM/Validation step was included in these workflows.

Concerning the DQM comparison, many of the plots have similar numbers of entries and similar shapes. I presented two plots where neither the number of entries nor the shapes match.

@slava77
Copy link
Contributor

slava77 commented Mar 22, 2016

On 3/22/16 4:00 PM, Carl Vuosalo wrote:

@slava77 https://github.com/slava77:
The wall-clock time difference is:

|Job total: 97.3583 s/ev ==> 96.9674 s/ev (-0.416 s/ev, -0.4%) |

this includes mixing module and validation in the job and, because of
this, is not useful as a comparison.

The module time difference is:

|Total in detailed printout: 5951.88 ms/ev -> 5041.42 ms/ev delta:
-910.462 (-15%) |

This is a sum for modules with differences (>5% for heavier or >10% for
lighter).
There is a total printout per reconstruction_step in the line after.
What is it?

Note that the DQM/Validation step was included in these workflows.

Concerning the DQM comparison, many of the plots have similar numbers of
entries and similar shapes. I presented two plots where neither the
number of entries nor the shapes match.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#13753 (comment)

@cvuosalo
Copy link
Contributor

@slava77:
The totals for the reco step are:

Total times: 9593.26 ms/ev -> 8678.51 ms/ev      delta: -914.753 (-9.5%)

@slava77
Copy link
Contributor

slava77 commented Mar 23, 2016

On 3/22/16 4:15 PM, Carl Vuosalo wrote:

@slava77 https://github.com/slava77:
The totals for the reco step is:

|Total times: 9593.26 ms/ev -> 8678.51 ms/ev delta: -914.753 (-9.5%) |

Thank you.

So, it is indeed about 10% decrease.
Something rather welcome, if it wasn't loosing efficiency at low pt (if
I understood the slides from TRK POG meeting correctly).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#13753 (comment)

@cvuosalo
Copy link
Contributor

+1

For #13753 e4daef5

Adding quadruplet seeding using the new fourth layer of the 2017 pixel tracker. There should be no changes in standard workflows.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-03-21-1100 show no significant differences, as expected. An extended test of workflow 10224.0_TTbar_13 (which is a 2017 workflow) with 70 events against baseline CMSSW_8_1_X_2016-03-10-2300 also show no significant differences with the new algorithm disabled. Test results with the new algorithm are discussed above, and they are satisfactory.

@makortel
Copy link
Contributor Author

@slava77 Indeed this version suffers from efficiency loss at low pT. One symptom is migration of tracks from quadrupled steps to triplet steps (best visible in lowPt*Step plots e.g. in @cvuosalo's comment #13753 (comment)). I'm experimenting ways to recover the low-pT efficiency.

@mmusich
Copy link
Contributor

mmusich commented Mar 24, 2016

+1
AlCa packages affected marginally via Calibration/IsolatedParticles in which the useQuadrupletAlgo is switched off.

@slava77
Copy link
Contributor

slava77 commented Mar 28, 2016

@Martin-Grunewald @perrotta we need your signature here

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants