Make Hip mitigation configurable #15194

VinInn · 2016-07-13T17:53:25Z

The title say all.
in principle bitwise compatible with previous version...
but for cosmics

cmsbuild · 2016-07-13T17:53:50Z

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

It involves the following packages:

RecoTracker/CkfPattern
RecoTracker/MeasurementDet
RecoTracker/TrackProducer
TrackingTools/DetLayers
TrackingTools/KalmanUpdators
Validation/RecoTrack

@cvuosalo, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @battibass, @makortel, @abbiendi, @GiacomoSguazzoni, @jhgoh, @VinInn, @bellan, @HuguesBrun, @mschrode, @rovere, @wmtford, @gpetruc, @trocino, @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 · 2016-07-13T17:54:44Z

@cmsbuild , please test

cmsbuild · 2016-07-13T17:55:02Z

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

slava77 · 2016-07-13T19:19:53Z

jenkins tests are being done in IB after #15114 is merged. So, the expectation is to have no changes.

slava77 · 2016-07-13T19:36:39Z

speaking of configurability: what is the python level spell to make MinPtForHitRecoveryInGluedDet become a very large number to disable the mitigation?

cmsbuild · 2016-07-13T20:42:35Z

-1

Tested at: b23eb58

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

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

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test testRecoMETMETProducers had ERRORS

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

cmsbuild · 2016-07-13T21:27:23Z

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

VinInn · 2016-07-14T06:38:43Z

RecoTracker/CkfPattern/python/GroupedCkfTrajectoryBuilderP5_cff.py

@@ -14,7 +14,7 @@
 Chi2MeasurementEstimatorForP5.nSigma = 4.
 Chi2MeasurementEstimatorForP5.MaxDisplacement = 100
 Chi2MeasurementEstimatorForP5.MaxSagitta=-1
-
+Chi2MeasurementEstimatorForP5.MinPtForHitRecoveryInGluedDet=100000


an example of python spell
http://cmslxr.fnal.gov/dxr/CMSSW_8_0_5/search?q=MaxSagitta&case=true
gives you an idea of all the places where such an explicit spell can be cast

VinInn · 2016-07-14T10:27:31Z

test failed due to

Failed to open the file 'root://xrootd-cms.infn.it//store/relval/CMSSW_7_3_0_pre1/RelValZEE_13/GEN-SIM-RECO/PRE_LS172_V15-v1/00000/C2212C6F-EB59-E411-AB78-0025905B85EE.root'

slava77 · 2016-07-14T12:28:56Z

@VinInn
For this PR to be useful for RelVal, we need a single python customise function that would flip MinPtForHitRecoveryInGluedDet to a large number.
Could you please add it.
Thanks.

VinInn · 2016-07-14T12:32:13Z

@slava77 , sorry I do not know how to make such a thing...
@makortel , can you help?

slava77 · 2016-07-14T12:49:26Z

On 7/14/16 5:32 AM, Vincenzo Innocente wrote:

@slava77 https://github.com/slava77 , sorry I do not know how to make
such a thing...

Maybe this can inspire
https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/HLTrigger/Configuration/python/customizeHLTforCMSSW.py

the method would pick all chi2Estimators of a process and change their
pset values.

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

VinInn · 2016-07-14T12:54:17Z

On 14 Jul, 2016, at 2:49 PM, Slava Krutelyov notifications@github.com wrote:

Maybe this can inspire
https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/HLTrigger/Configuration/python/customizeHLTforCMSSW.py

the method would pick all chi2Estimators of a process and change their
pset values.

and where you suggest such a customize to reside?
v.

cmsbuild · 2016-07-15T15:36:59Z

-1

Tested at: 311d80d

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

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test testRecoMETMETProducers had ERRORS

cmsbuild · 2016-07-15T16:17:35Z

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

slava77 · 2016-07-15T16:54:42Z

Looking at jenkins results, the comparisons look as expected now (conversions didn't change, only cosmics changes).
Only a fraction of regular workflow comparisons made it through though.
So, I'm checking locally.

slava77 · 2016-07-15T21:25:46Z

RecoTracker/Configuration/python/customizeMinPtForHitRecoveryInGluedDet.py

+    return (module for module in process._Process__esproducers.values() if module._TypedParameterizable__type in types)
+def customizeMinPtForHitRecoveryInGluedDet(process,value):
+   for esp in esproducers_by_type(process, "Chi2MeasurementEstimatorESProducer", "Chi2ChargeMeasurementEstimatorESProducer"):
+       esp.MinPtForHitRecoveryInGluedDet = value


please change this to

esp.MinPtForHitRecoveryInGluedDet = cms.double(value)

so that this customization applies as is to HLT and possibly other PSets that were not derived from a clone.
If done, also add

import FWCore.ParameterSet.Config as cms

at the top

With these modifications the function can be tested with --customise RecoTracker/Configuration/customizeMinPtForHitRecoveryInGluedDet.customizeHitRecoveryInGluedDetOff passed to the cmsDriver.py or runTheMatrix.py

This is intentional.
we want to catch those who do not clone.
They HAVE to clone.

For what concern HLT, my understanding is that as soon this PR will be integrated a new config will be created with the new parameter made explicit.
@mtosi, @Martin-Grunewald, @fwyzard to confirm

I think the HLT policy for new PRs is to support the old/current menu.
So, missing required parameters should be inserted with a customization.
This is a bit special case, because we are discussing the customization function which is not a standard workflow.

relval submission that calls this customization will be rather complicated, each workflow type will need to be handled manually (not all have HLT); although the majority has RECO in step3. So, should likely be OK.

This customise if not modified will not work for fastsim: ~OK, probably.

ok let's modified.
FastSim does not perform patternreco so any value is ok

Fix it so that it is added also in non-clones

cmsbuild · 2016-07-16T15:05:35Z

Pull request #15194 was updated. @cvuosalo, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

VinInn · 2016-07-16T15:05:50Z

@cmsbuild , please test

cmsbuild · 2016-07-16T15:06:00Z

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

cmsbuild · 2016-07-16T17:04:43Z

-1

Tested at: 93dbb05

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

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test testRecoMETMETProducers had ERRORS

cmsbuild · 2016-07-16T17:46:34Z

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

slava77 · 2016-07-17T01:23:11Z

+1

for #15194 93dbb05

HIP mitigation is made configurable for cases where Chi2 estimator is taken from event setup. Also, the cosmic track reco mitigation logic is disabled. The customization function is added, which would revert the pt threshold of 0.9 GeV used for mitigation in the configurable cases; few other cases where the estimator is instantiated manually the default (mitigation) threshold will remain. So, the customizeHitRecoveryInGluedDetOff gives a result very close to pre-mitigation, but not identical.
jenkins tests pass and show differences only in cosmic tracking
local tests on higher stats show that the default config gives results consistent with jenkins tests.
Tests done with the customizeHitRecoveryInGluedDetOff call recovers pre-Hip mitigation #15114 behavior with a pretty good agreement. Few discrepancies remain as expected (changes in conversions are expected; rare changes in general tracks can happen too if some trajectories pt go above 1E5 GeV). The baseline for the tests was 810pre8 as pre-Hip mitigation #15114; 810pre8+Hip mitigation #15114 for the equivalent to jenkins tests baseline, and 810pre8+Hip mitigation #15114+thisPR (cherry-picks) for this PR tests.
- this is somewhat good enough for a separate relval if there is need to decouple the HIP mitigation changes from other physics changes in the pre-release.
- technically, this is not good enough for a verbatim backport to 80X if one is needed for a purpose of testing HIP mitigation enabled. If a backport is done, it would need to be at least with the mitigation PT default changed to double.max and an inverse customization available [ customizeHitRecoveryInGluedDetOn ]

slava77 · 2016-07-17T01:58:12Z

+1
maybe this works better than #15194 (comment)

mtosi · 2016-07-18T08:24:02Z

do we have the analogous for 80x ?
@VinInn @perrotta @Martin-Grunewald

VinInn added 4 commits July 13, 2016 19:18

make hip mitigation configurable

f9b1eae

in glued det as well

9dacf1b

Merged HipMitigation2 from repository VinInn

c06ea3f

factorize params

b23eb58

cmsbuild added this to the Next CMSSW_8_1_X milestone Jul 13, 2016

cmsbuild added reconstruction-pending dqm-pending pending-signatures tests-pending orp-pending comparison-pending labels Jul 13, 2016

cmsbuild added tests-started and removed tests-pending labels Jul 13, 2016

cmsbuild added tests-rejected and removed tests-started labels Jul 13, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jul 13, 2016

VinInn reviewed Jul 14, 2016
View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Jul 15, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jul 15, 2016

slava77 reviewed Jul 15, 2016
View reviewed changes

Update customizeMinPtForHitRecoveryInGluedDet.py

93dbb05

Fix it so that it is added also in non-clones

cmsbuild added tests-pending comparison-pending and removed comparison-available tests-rejected labels Jul 16, 2016

cmsbuild added tests-rejected and removed tests-pending labels Jul 16, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jul 16, 2016

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jul 17, 2016

davidlange6 merged commit bb4c80e into cms-sw:CMSSW_8_1_X Jul 18, 2016

makortel mentioned this pull request Jul 21, 2016

Backport HIP mitigation to 80X #15251

Merged

makortel mentioned this pull request Aug 24, 2016

No vfp30 mitigation in Tracking (including customization with era) #15590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Hip mitigation configurable #15194

Make Hip mitigation configurable #15194

VinInn commented Jul 13, 2016 •

edited

cmsbuild commented Jul 13, 2016

VinInn commented Jul 13, 2016

cmsbuild commented Jul 13, 2016 •

edited

slava77 commented Jul 13, 2016

slava77 commented Jul 13, 2016

cmsbuild commented Jul 13, 2016

cmsbuild commented Jul 13, 2016

VinInn Jul 14, 2016

VinInn commented Jul 14, 2016

slava77 commented Jul 14, 2016

VinInn commented Jul 14, 2016 •

edited

slava77 commented Jul 14, 2016

VinInn commented Jul 14, 2016

cmsbuild commented Jul 15, 2016

cmsbuild commented Jul 15, 2016

slava77 commented Jul 15, 2016

slava77 Jul 15, 2016

VinInn Jul 16, 2016

VinInn Jul 16, 2016

slava77 Jul 16, 2016

slava77 Jul 16, 2016

slava77 Jul 16, 2016

VinInn Jul 16, 2016

cmsbuild commented Jul 16, 2016

VinInn commented Jul 16, 2016

cmsbuild commented Jul 16, 2016 •

edited

cmsbuild commented Jul 16, 2016

cmsbuild commented Jul 16, 2016

slava77 commented Jul 17, 2016 •

edited

slava77 commented Jul 17, 2016

mtosi commented Jul 18, 2016

Make Hip mitigation configurable #15194

Make Hip mitigation configurable #15194

Conversation

VinInn commented Jul 13, 2016 • edited

cmsbuild commented Jul 13, 2016

VinInn commented Jul 13, 2016

cmsbuild commented Jul 13, 2016 • edited

slava77 commented Jul 13, 2016

slava77 commented Jul 13, 2016

cmsbuild commented Jul 13, 2016

cmsbuild commented Jul 13, 2016

Choose a reason for hiding this comment

VinInn commented Jul 14, 2016

slava77 commented Jul 14, 2016

VinInn commented Jul 14, 2016 • edited

slava77 commented Jul 14, 2016

VinInn commented Jul 14, 2016

cmsbuild commented Jul 15, 2016

cmsbuild commented Jul 15, 2016

slava77 commented Jul 15, 2016

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

cmsbuild commented Jul 16, 2016

VinInn commented Jul 16, 2016

cmsbuild commented Jul 16, 2016 • edited

cmsbuild commented Jul 16, 2016

cmsbuild commented Jul 16, 2016

slava77 commented Jul 17, 2016 • edited

slava77 commented Jul 17, 2016

mtosi commented Jul 18, 2016

VinInn commented Jul 13, 2016 •

edited

cmsbuild commented Jul 13, 2016 •

edited

VinInn commented Jul 14, 2016 •

edited

cmsbuild commented Jul 16, 2016 •

edited

slava77 commented Jul 17, 2016 •

edited