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

Hip mitigation #15114

Merged
merged 11 commits into from Jul 12, 2016
Merged

Hip mitigation #15114

merged 11 commits into from Jul 12, 2016

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Jul 5, 2016

HIP Mitigation as extensively presented and discussed.
See for instance:
https://indico.cern.ch/event/539401/contributions/2207119/attachments/1296862/1934523/HipMitigation3.pdf

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2016

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_8_1_X.

It involves the following packages:

RecoTracker/MeasurementDet

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @mschrode, @gpetruc, @dgulhan 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

@VinInn
Copy link
Contributor Author

VinInn commented Jul 5, 2016

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2016

@slava77
Copy link
Contributor

slava77 commented Jul 5, 2016

@cmsbuild please test
the last comparisons run failed in rsync

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2016

-1

Tested at: d5168db

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

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
136.731 step1

DAS Error
140.53 step1
DAS Error

@VinInn
Copy link
Contributor Author

VinInn commented Jul 8, 2016

GSF tracking parameters had never been tuned
http://cmslxr.fnal.gov/lxr/source/TrackingTools/GsfTracking/python/CkfElectronCandidateMaker_cff.py?v=CMSSW_8_1_X_2016-07-06-2300

maybe EGamma POG should have a look and not just set all cuts to ~inf : at some point junk kicks in and efficiency is lost...

@slava77
Copy link
Contributor

slava77 commented Jul 8, 2016

@rafaellopesdesa @fcouderc
something for you to note (#15114 (comment) and earlier discussion here)

@slava77
Copy link
Contributor

slava77 commented Jul 12, 2016

On 7/6/16 11:57 PM, Vincenzo Innocente wrote:

PS
275828 is declared BAD by Tracker because of large inefficiencies in TID
due to noise.
The fact that this PR happens to mitigate also that inefficiency will
not change the decision....

I missed this.
Rerunning with something with a similar PU and apparently good (276092)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15114 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/AEdcbvAeBBNzoLWPTEO5TGeUAKVHCqn-ks5qTKNwgaJpZM4JE9VO.

@VinInn
Copy link
Contributor Author

VinInn commented Jul 12, 2016

In any case "somebody" has to decide

  1. cut a (special)-release, make relvals and ask POGs to validate
  2. ask to neutralize GSF and conversions (as done in the past for other optimizations) and then proceed with 1)

@slava77
Copy link
Contributor

slava77 commented Jul 12, 2016

I tried with Simon's simulation PR and see somewhat similar behavior in data and MC.

Here is a comparison of JetHT 276092 with MC ttbar PU 35 (with HIP simulation scaled to PU35; APVSaturationProbScaling = cms.double(1.34) )

data: {collection: change (count of objects in original, comments}

    muons + 7.6% (7276 mostly bad TMID
    glob mu tks: +2.4%
    particleFlowEgamma: -16% (7719
    gsfTracks: - 24% (14726
    pf ch +0.5%
    pf ele: -18% (989
    pf mu: +11% (1175
    CDC: +42% (166
    generalTracks: +2.3% (732585
    IVFs: +49% (1616
    gedGsfEle: -11% (1467;
    gedPhotons: -18% (3022; mostly at low E

MC

    muons: +14% mostly with bad TMID
    globalMuonTracks: +1: 74->75
    ele gsf tk: -27% (974
    particleFlowEgamma -20% (577
    pf ch: +0.2% (50682; mostly around 1 GeV
    pf el: -18% (101
    pf mu: +16% (151
    generalTracks +1.4% (61927
    IVFs + 50% (132
    gedGsfEle: -15% (148
    gedPhotons -20% (267

If I can trust MC plots with HIP simulation on particle gun events and on high-pt dijets,
then there isn't much visible effect on isolated electrons and photons (for guns), and not much of an effect from jetCore behavior in high-pt dijets.

Some plots for comparisons:
pfEgamma in data
all_sign736vsorig_jetht276092c_log10recopfcandidates_particleflowegamma__reco_obj_pt
and in MC:
all_sign736s-pass-af58560vstest15062-pass-af58560_ttbarpuwf25202p0c_log10recopfcandidates_particleflowegamma__reco_obj_pt

GED GSF data
all_sign736vsorig_jetht276092c_recogsfelectrons_gedgsfelectrons__reco_obj_classification
and MC
all_sign736s-pass-af58560vstest15062-pass-af58560_ttbarpuwf25202p0c_recogsfelectrons_gedgsfelectrons__reco_obj_classification

gedPhotons data
all_sign736vsorig_jetht276092c_recophotons_gedphotons__reco_obj_et
and MC
all_sign736s-pass-af58560vstest15062-pass-af58560_ttbarpuwf25202p0c_recophotons_gedphotons__reco_obj_et

TTbar MC for these inclusive plots is still rather dominated by non-W leptons and fake photons.
So, the similarity in changes from the mitigation are encouraging.

@slava77
Copy link
Contributor

slava77 commented Jul 12, 2016

Here are some technical numbers, based on 276092 JetHT
(I was running 8-threads on 1000 events)

There is about 1s/event increase (about 5% of total CPU).
The largest contributors are in tracking:

   -0.392600      -0.10%        56.29 ms/ev ->        37.82 ms/ev pfTrackElec
   -0.295850      -0.27%       193.69 ms/ev ->       143.77 ms/ev electronGsfTracks
   +0.192923      +0.16%       135.01 ms/ev ->       163.84 ms/ev particleFlowDisplacedVertex
   -0.190227      -0.08%        86.89 ms/ev ->        71.80 ms/ev allConversions
   +0.132258      +0.14%       181.57 ms/ev ->       207.28 ms/ev unsortedOfflinePrimaryVertices
   +0.071275      +0.07%       177.71 ms/ev ->       190.84 ms/ev earlyDisplacedMuons
   +0.070947      +0.07%       176.74 ms/ev ->       189.73 ms/ev earlyMuons
   +0.047114      +0.21%       813.70 ms/ev ->       852.96 ms/ev muons1stStep
   -0.064891      -0.05%       154.73 ms/ev ->       145.00 ms/ev trackExtrapolator
   +0.046377      +0.05%       196.45 ms/ev ->       205.78 ms/ev conversionTrackCandidates

   +0.281431      +1.21%       678.74 ms/ev ->       901.04 ms/ev jetCoreRegionalStepTrackCandidates
   +0.257997      +2.01%      1247.71 ms/ev ->      1617.29 ms/ev pixelPairStepTrackCandidates
   +0.232694      +0.06%        43.27 ms/ev ->        54.67 ms/ev pixelPairStep
   +0.160113      +0.37%       392.62 ms/ev ->       460.96 ms/ev pixelPairStepTracks
   +0.121924      +0.03%        49.29 ms/ev ->        55.69 ms/ev firstStepPrimaryVerticesPreSplitting
   +0.118777      +0.04%        53.46 ms/ev ->        60.21 ms/ev firstStepPrimaryVertices
   +0.115772      +0.03%        50.09 ms/ev ->        56.25 ms/ev earlyGeneralTracks
   +0.107825      +0.40%       650.55 ms/ev ->       724.70 ms/ev detachedTripletStepTrackCandidates
   -0.086483      -0.04%        86.31 ms/ev ->        79.15 ms/ev tobTecStepSeedsTripl
   +0.084617      +0.24%       496.86 ms/ev ->       540.76 ms/ev initialStepTrackCandidatesPreSplitting
   +0.084475      +0.13%       272.08 ms/ev ->       296.08 ms/ev pixelLessStepTrackCandidates
   +0.080211      +0.23%       497.29 ms/ev ->       538.85 ms/ev initialStepTrackCandidates
   +0.078741      +0.03%        68.01 ms/ev ->        73.59 ms/ev pixelLessStepTracks
   +0.069524      +0.24%       609.70 ms/ev ->       653.61 ms/ev lowPtTripletStepTrackCandidates
   +0.039383      +0.04%       204.66 ms/ev ->       212.88 ms/ev lowPtTripletStepTracks
   -0.034641      -0.09%       473.37 ms/ev ->       457.25 ms/ev pixelLessStepSeeds
   +0.025208      +0.04%       271.02 ms/ev ->       277.94 ms/ev detachedTripletStepTracks
Total printed: 8317.81 -> 9277.74

The event sizes go up and down per product. The net change is about 1%, mainly coming from generalTracks. Changes in the reco file:

    821.4 ->       724.9        -96    -12.5  -0.01     recoPFCandidates_particleFlowEGamma__RECO.
     81.0 ->       106.9         26     27.5   0.00     recoTracks_cosmicsVetoTracks__RECO.
   3362.4 ->      3615.8        253      7.3   0.01     recoMuons_muons__RECO.
  79518.1 ->     81801.3       2283      2.8   0.13     recoTracks_generalTracks__RECO.
   2368.7 ->      1866.6       -502    -23.7  -0.03     recoGsfTracks_electronGsfTracks__RECO.
   1053.4 ->       966.1        -87     -8.6  -0.00     recoGsfElectrons_gedGsfElectrons__RECO.
   1232.2 ->      1054.9       -177    -15.5  -0.01     recoPhotons_gedPhotons__RECO.
  60173.7 ->     60149.9        -24     -0.0  -0.00     recoPFCandidates_particleFlow__RECO.
    727.2 ->       985.1        258     30.1   0.01     recoVertexs_inclusiveSecondaryVertices__RECO.

@slava77
Copy link
Contributor

slava77 commented Jul 12, 2016

+1

for #15114 ce75214

  • code change is as described
  • jenkins tests pass and show changes in many workflows
  • local tests with HIP simulation from HIP simulation in SiStrips #15062 and with JetHT 276092 (and 275828 earlier, which is a bit problematic in TID, but has fairly similar changes from this PR) show changes in line with the presentation linked in the description for general iterative tracks; changes in higher level objects appear to be somewhat reproducible in MC (larger effects on fake-like leptons and photons).

@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

@VinInn
Copy link
Contributor Author

VinInn commented Jul 12, 2016

Thanks Slava for these tests: you are indeed the first to try this new HIP simulation/mitigation chain!
We really need higher stat
@arizzi fyi

@arizzi
Copy link
Contributor

arizzi commented Jul 12, 2016

@VinInn is it possible to make a 80X backport to possibly have a dedicated 80X patch release to do a selected rereco of some runs/PDs ?

@VinInn
Copy link
Contributor Author

VinInn commented Jul 12, 2016

why in 80X?
the difference is so large that at this point is a joke to pretend bitwise compatibility in other areas...

btw we then need also a backport of HIP Simulation...

@arizzi
Copy link
Contributor

arizzi commented Jul 12, 2016

it would make easy to compare w/ prompt reco and faster to prepare the campaign given that GT is already there

@slava77
Copy link
Contributor

slava77 commented Jul 12, 2016

On 7/12/16 8:27 AM, arizzi wrote:

@VinInn https://github.com/VinInn is it possible to make a 80X
backport to possibly have a dedicated 80X patch release to do a selected
rereco of some runs/PDs ?

If this can be made configurable for 80X (or even 81X), off by default
in 80X,
then backport and special reprocessing requests in 80X are going to be
transparent.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15114 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/AEdcbtEknKGiFd4tMQ3c2c7n-FnGD_0Iks5qU7JIgaJpZM4JE9VO.

\

@VinInn
Copy link
Contributor Author

VinInn commented Jul 12, 2016

a straight backport is trivial (a few line patch)
I was planning to add configurability using "MeasurementEstimator" as a carrier.
This will take a while and will require the usual HLT gym.
I still suggest to create a special release for HIPMitigation at this round....

@slava77
Copy link
Contributor

slava77 commented Jul 12, 2016

On 7/12/16 9:06 AM, Vincenzo Innocente wrote:

a straight backport is trivial (a few line patch)
I was planning to add configurability using "MeasurementEstimator" as a
carrier.
This will take a while and will require the usual HLT gym.
I still suggest to create a special release for HIPMitigation at this
round....

It should be possible to do all the tests and comparisons in 81X,
as long as the idea is not to reprocess O(10%) of all data at which
point doing in 80X gives some savings.

The development of things appears to go in the direction of having the
mitigation in rereco.
That's probably still 80X.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15114 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/AEdcbvOvDvkU0YPQcCXrV5-5LJbs1Jlbks5qU7uKgaJpZM4JE9VO.

@VinInn
Copy link
Contributor Author

VinInn commented Jul 12, 2016

So, what shall we do?

  1. backport this?
  2. integrate this in 81X (possibly asap)?
  3. hold and add configurability?

btw keep in mind that we still hope to be able to identify affected APVs from data themselves and make the mitigation more local

@venturia fyi as well

@slava77
Copy link
Contributor

slava77 commented Jul 12, 2016

On 7/12/16 9:53 AM, Vincenzo Innocente wrote:

So, what shall we do?

  1. backport this?

No, only when it's configurable.

  1. integrate this in 81X (possibly asap)?

  2. hold and add configurability?
    I think that on/off switch is helpful for 81X validation as well
    (given more physics changes coming in the next pre-release).
    If I understand correctly, this can be implemented in about a day,
    which is on schedule for the next pre-release.

btw keep in mind that we still hope to be able to identify affected APVs
from data themselves and make the mitigation more local

What is the time scale for this?

@venturia https://github.com/venturia fyi as well


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15114 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/AEdcbgRr7dmda8wumDByiFxcVwfZ_ZSFks5qU8aUgaJpZM4JE9VO.

@VinInn
Copy link
Contributor Author

VinInn commented Jul 12, 2016

the new parameter will make the pair with this for instance http://cmslxr.fnal.gov/dxr/CMSSW_8_0_5/search?q=MaxSagitta&case=true
what we prefer: neutralize GSF and conversions or not? (muon steps for sure not!)

what what concern APV identification: no-ETA

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 7ff241d into cms-sw:CMSSW_8_1_X Jul 12, 2016
@slava77
Copy link
Contributor

slava77 commented Jul 12, 2016

On 7/12/16 10:34 AM, Vincenzo Innocente wrote:

the new parameter will make the pair with this for instance
http://cmslxr.fnal.gov/dxr/CMSSW_8_0_5/search?q=MaxSagitta&case=true

what we prefer: neutralize GSF and conversions or not? (muon steps for
sure not!)

I may be misunderstanding the term "neutralize".

At this point I was just thinking of a global on/off switch for the
mitigation in this PR

Your comments point to further improvements that would run differently
for different iterations.
This is good, but I'm somewhat out of the context.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15114 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/AEdcboDEA8BCCh7R1_XiQjgrAMdw3Ibxks5qU9APgaJpZM4JE9VO.

\

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