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

Add automated pixel pair mitigation for inactive pixel regions #21630

Merged
merged 34 commits into from Dec 14, 2017

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Dec 1, 2017

This PR adds automated pixel pair mitigation for regions in pixel where there are inactive areas in two layers.

The algorithm has been presented in tracking POG meeting on November 6th
https://indico.cern.ch/event/675097/contributions/2775830/attachments/1553214/2442464/updated_slides_trk_mk_20171106.pdf
A notable addition to the version presented is an option to ignore single inactive panels in FPix from the inactive areas. This option reduces fake rate without notable change in efficiency. Here are MTV plots comparing 1k ttbar+PU50 events in 940pre3 for the pre-release, the "POG meeting version", and the version ignoring single inactive panels
https://cms-tracking-validation.web.cern.ch/cms-tracking-validation/PR/CMSSW_9_4_0_pre3_PR21630/

The algorithm is implemented in PixelInactiveAreaTrackingRegionsSeedingLayersProducer, which produces

  • SeedingLayerSetsHits (normally produced by SeedingLayersEDProducer)
    • Technically could be produced with SeedingLayersEDProducer here as well, but then I'd need new accessors for some details in SeedingLayerSetsHits, and I felt that the current solution would be a bit cleaner
  • TrackingRegionsSeedingLayerSets, which is essentially vector<TrackingRegion, vector<SeedingLayerSet>> i.e. for each region holds also the seeding layer pair to be used for that region.
    The producer is further split in two parts
  • PixelInactiveAreaFinder
    • Finds the inactive modules on each layer and group adjacent inactive modules together
    • Looks for layer pairs that have overlapping inactive areas and returns them and the list of layer pairs that should be used in seeding
  • AreaSeededTrackingRegionsBuilder
    • Creates RectangularEtaPhiTrackingRegion for a given vector of (phi, r, z) boxes such that the eta-phi window covers all the boxes

As a "byproduct"

  • created AreaSeededTrackingRegionsProducer
    • uses also AreaSeededTrackingRegionsBuilder to create RectangularEtaPhiTrackingRegions, but the input (phi, r, z) boxes come from configuration
    • (actually I created this producer as one of the first steps even if it is not strictly needed for this PR, but it could be useful in the future)
  • abstracted the vertex/beamspot origins to their own class
    • common part with CandidatePointSeededTrackingRegionsProducer
  • in tracking DQM
    • monitor eta-phi areas covered by RectangularEtaPhiTrackingRegions
      • currently we monitor only the direction
      • with variable-size regions monitoring also the covered area becomes interesting, and also allows to correlate regions wrt. pixel maps
    • in the configuration of "track building and seeding monitoring" replace the dictionaries with cms.PSets to be able to customize them with eras
      • there is further room to reduce copy-paste and harmonize naming of PSet members with the TrackingMonitor parameters

Enabling the code in reconstruction is left to a subsequent PR. In the mean time the following commits can be used on top of this PR to enable the mitigation.
makortel/cmssw@areaSeededRegion_step1...areaSeededRegion_step2

Tested in CMSSW_10_0_X_2017-11-27-2300 (rebased to CMSSW_10_0_0_pre2), no changes expected expecting changes in phase1 workflows as shown in the MTV plots above.

@VinInn @nikoleskinen @JanFSchulte @mtosi

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2017

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2017

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21630/2395

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-21630/2395/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-21630/2395/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2017

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2017

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21630/2401

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2017

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

It involves the following packages:

DQM/TrackingMonitor
DQM/TrackingMonitorSource
RecoTracker/TkHitPairs
RecoTracker/TkSeedingLayers
RecoTracker/TkTrackingRegions
TrackingTools/TransientTrackingRecHit
Validation/RecoTrack

@perrotta, @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @slava77, @vanbesien can you please review it and eventually sign? Thanks.
@ghellwig, @felicepantaleo, @abbiendi, @fioriNTU, @threus, @hdelanno, @battibass, @jhgoh, @HuguesBrun, @trocino, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @wmtford, @mschrode, @idebruyn, @ebrondol, @mtosi, @dgulhan, @calderona, @gpetruc 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
Copy link
Contributor Author

makortel commented Dec 1, 2017

@cmsbuild, please test

I also added link to MTV plots showing the effect of this feature (when enabled) to the PR description.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24914/console Started: 2017/12/01 22:26

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2017

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

There are some workflows for which there are errors in the baseline:
10824.0 step 5
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2606833
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2606661
  • DQMHistoTests: Total skipped: 171
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 68.8599999999 KiB( 22 files compared)
  • Checked 106 log files, 8 edm output root files, 26 DQM output files

@cmsbuild
Copy link
Contributor

Pull request #21630 was updated. @perrotta, @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @slava77, @vanbesien can you please check and sign again.

@makortel
Copy link
Contributor Author

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 12, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25062/console Started: 2017/12/12 20:07

@cmsbuild
Copy link
Contributor

@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-21630/25062/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-21630/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1529 differences found in the comparisons
  • DQMHistoTests: Total files compared: 27
  • DQMHistoTests: Total histograms compared: 2835241
  • DQMHistoTests: Total failures: 890
  • DQMHistoTests: Total nulls: 1356
  • DQMHistoTests: Total successes: 2832817
  • DQMHistoTests: Total skipped: 178
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 159.76 KiB( 22 files compared)
  • Checked 113 log files, 9 edm output root files, 27 DQM output files

@dmitrijus
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Dec 13, 2017

+1

for #21630 39f3ff2

  • changes are in line with the PR description: aiming to improve pixel pair recovery by making regions for pixelPairStep based on the information known from the conditions or bad ROC data event by event. This affects only phase 1.
  • jenkins tests pass and comparisons with baseline show somewhat small changes in phase-1 workflows starting from tracking variables and propagating downstream
  • local tests confirm expected behavior

at the track level there is some increase in efficiency in bad regions of eta/phi
wfmatti_ttbarpu50_efffake_eta

the effect is visible more clearly in a run with a larger fraction of bad modules, e.g. 305064 (wf 136.829
In this run the bad regions can be seen in the z-phi or xy plots (picked from offline DQM)
run305064_dqm_clupos_bpix_zphi

run305064_dqm_clupos_fpix_zphi

Here the pixelPair seeding with this PR is more "selective" around the partially dead regions
(left is baseline, right is this PR)
wf136 829_pixpair_seeds_etaphi
this eventually propagates to the generalTracks level with about 10% increase in the pixelPair algo tracks and even some decrease in pixelLess
all_sign984vsorig_rundoubleeg2017fwf136p829c_recotracks_generaltracks__rereco_obj_algo

High level quantities are not affected significantly, mostly because the effect is rather small.

CPU per event in reco has decreased by about 1-2% in 10224 (somewhat significant above general measurement uncertainty) and no clear change in 2017F data workflows in (up by about 1% in 136.829/DoubleEG and down by about 1% in 136.83/DoubleMuon).

@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 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)

@JanFSchulte
Copy link
Contributor

For completeness sake: This new recovery procedure has also been tested, see the report at the TSG workshop yesterday: https://indico.cern.ch/event/674023/contributions/2805626/attachments/1574212/2485184/20171212_TSGWorkshop_Tracking.pdf

It certainly has the potential to recover a lot of lost efficiency there, but the procedure needs a bit of refining going forward to keep the timing under control.

@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

7 participants