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

Fast match #2581

Merged
merged 8 commits into from Feb 28, 2014
Merged

Fast match #2581

merged 8 commits into from Feb 28, 2014

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Feb 21, 2014

these modification enable the possibility to study a "faster" version of the hit matcher
that preselects the hits to match based on the track parameters
The selection is controlled by a parameter of the Matcher "PreFilter"
default is false.
to set to true just

--- a/RecoLocalTracker/SiStripRecHitConverter/python/SiStripRecHitMatcher_cfi.py
+++ b/RecoLocalTracker/SiStripRecHitConverter/python/SiStripRecHitMatcher_cfi.py
@@ -3,7 +3,7 @@ import FWCore.ParameterSet.Config as cms
 SiStripRecHitMatcherESProducer = cms.ESProducer("SiStripRecHitMatcherESProducer",
     ComponentName = cms.string('StandardMatcher'),
     NSigmaInside = cms.double(3.0),
-    PreFilter = cms.untracked.bool(False)
+    PreFilter = cms.untracked.bool(True)
 )

the speed up obtained depends on occupancy and track density in each det-unit.
At high pileup it is expected to be significant.
for a typical JetHT sample from 2012C one saves about 25% of the time in the matcher itself
corresponding to about a 6% speed up of the total CkfTrackCandidateMakerBase.

the cost is a small inefficiency caused by a loss of about 10% of the hits on the track, as it can be seen in
the standard MTV plots for TTBar
http://innocent.home.cern.ch/innocent/FastMatchMTV/

this loss in not present al all in single particle events and seems to be constant with the increase of the pileup

The use of this optimization shall be therefore reviewed in light of the final configuration for Run 2
including cluster-charge cuts and new setup for the iterative-tracking

I have of course verified that the modifications with default configuration do not produce any regression

@slava77
Copy link
Contributor

slava77 commented Feb 21, 2014

Hi Vincenzo,
which TTBar was it in your link?

@slava77
Copy link
Contributor

slava77 commented Feb 21, 2014

Since the performance changes on the physics side, I think that we need this vetted by the tracking POG.

I'd expect that PU40@25ns at 13 TeV, our baseline for post-LS1 should be included in the tests for
something like TTbar and Zmumu.

@VinInn
Copy link
Contributor Author

VinInn commented Feb 21, 2014

202.0_TTbar+TTbarINPUT+DIGIPU1+RECOPU1+HARVEST
with PreFilter on of course!

Prefilter off there is no regression even with your validate.C
I cannot show you the plot because it produces none...

Tracking POG is fully informed and we decided for this route.
We need this mods to change just a Param in config and not edit the code..

NO regression, at all with the committed code.

I have PU40 TTbar . I have to run them again starting from step2...

@cmsbuild
Copy link
Contributor

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

Fast match

It involves the following packages:

RecoLocalTracker/SiStripRecHitConverter
RecoTracker/MeasurementDet

@nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please review it and eventually sign? Thanks.
@gpetruc, @cerati, @ghellwig, @GiacomoSguazzoni, @rovere this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@VinInn
Copy link
Contributor Author

VinInn commented Feb 21, 2014

On 21 Feb, 2014, at 3:36 PM, slava77 notifications@github.com wrote:

Since the performance changes on the physics side, I think that we need this vetted by the tracking POG.
sure I expect @rovere and @cerati to comment soon.

v.

@slava77
Copy link
Contributor

slava77 commented Feb 21, 2014

OK, if it's just adding the ability to cut, but not cut, this PR itself can get in quicker ;)

@ktf Giulio, the bot was pretty sluggish here, it took it almost an hour to notice this PR

@cmsbuild
Copy link
Contributor

@nclopezo nclopezo modified the milestones: CMSSW_7_1_0_pre4, CMSSW_7_1_0_pre3 Feb 24, 2014
@VinInn
Copy link
Contributor Author

VinInn commented Feb 25, 2014

Sorry, what is holding progress toward approval here?

@davidlange6
Copy link
Contributor

@VinInn, this new parameter would seem to break hlt configurations, no? (not all of them I guess)

@VinInn
Copy link
Contributor Author

VinInn commented Feb 25, 2014

is untracked. I know HLT likes to have all parameters specified, but will not break the config.
in any case there is only one instance of it
grep SiStripRecHitMatcherESProducer OnData_HLT_GRun.py
process.SiStripRecHitMatcherESProducer = cms.ESProducer( "SiStripRecHitMatcherESProducer",

@mtosi can comment and eventually act

Is untracked exactly not to break configs.
Of course if we go in production with it, it needs to be made tracked as it modifies the products

@davidlange6
Copy link
Contributor

its untracked, but the code is requiring it to be in the pset, no?

On Feb 25, 2014, at 12:12 PM, Vincenzo Innocente notifications@github.com
wrote:

is untracked. I know HLT likes to have all parameters specified, but will not break the config.
in any case there only one instance of it
grep SiStripRecHitMatcherESProducer OnData_HLT_GRun.py
process.SiStripRecHitMatcherESProducer = cms.ESProducer( "SiStripRecHitMatcherESProducer",

@mtosi can comment and eventually act


Reply to this email directly or view it on GitHub.

@VinInn
Copy link
Contributor Author

VinInn commented Feb 25, 2014

really?
my understanding of the statement

 preFilter_(conf.getUntrackedParameter<bool>("PreFilter",false)) 

has always been "if missing is false"

unmodified HLT config
grep SiStripRecHitMatcherESProducer -A10 OnData_HLT_GRun.py
process.SiStripRecHitMatcherESProducer = cms.ESProducer( "SiStripRecHitMatcherESProducer",
ComponentName = cms.string( "StandardMatcher" ),
NSigmaInside = cms.double( 3.0 )
)
is running happily
cmsRun OnData_HLT_GRun.py
globaltag = GR_H_V34::All
410 DQMStore::DQMStore
25-Feb-2014 12:27:07 CET Initiating request to open file file:run207515_lumi183.root
25-Feb-2014 12:27:07 CET Initiating request to open file file:run207515_lumi183.root
25-Feb-2014 12:27:07 CET Successfully opened file file:run207515_lumi183.root
25-Feb-2014 12:27:07 CET Successfully opened file file:run207515_lumi183.root
MSLayersKeeperX0DetLayer LAYERS:
25-Feb-2014 12:29:34 CET Closed file file:run207515_lumi183.root
25-Feb-2014 12:29:34 CET Closed file file:run207515_lumi183.root
25-Feb-2014 12:29:34 CET Initiating request to open file file:run207515_lumi184.root
25-Feb-2014 12:29:34 CET Initiating request to open file file:run207515_lumi184.root
25-Feb-2014 12:29:34 CET Successfully opened file file:run207515_lumi184.root
25-Feb-2014 12:29:34 CET Successfully opened file file:run207515_lumi184.root

@davidlange6
Copy link
Contributor

Ha. Missed the default argument. Sorry

On Feb 25, 2014, at 12:32 PM, "Vincenzo Innocente" <notifications@github.commailto:notifications@github.com> wrote:

really?
my understanding of the statement

preFilter_(conf.getUntrackedParameter("PreFilter",false))

has always been "if missing is false"

unmodified HLT config
grep SiStripRecHitMatcherESProducer -A10 OnData_HLT_GRun.py
process.SiStripRecHitMatcherESProducer = cms.ESProducer( "SiStripRecHitMatcherESProducer",
ComponentName = cms.string( "StandardMatcher" ),
NSigmaInside = cms.double( 3.0 )
)
is running happily
cmsRun OnData_HLT_GRun.py
globaltag = GR_H_V34::All
410 DQMStore::DQMStore
25-Feb-2014 12:27:07 CET Initiating request to open file file:run207515_lumi183.root
25-Feb-2014 12:27:07 CET Initiating request to open file file:run207515_lumi183.root
25-Feb-2014 12:27:07 CET Successfully opened file file:run207515_lumi183.root
25-Feb-2014 12:27:07 CET Successfully opened file file:run207515_lumi183.root
MSLayersKeeperX0DetLayer LAYERS:
25-Feb-2014 12:29:34 CET Closed file file:run207515_lumi183.root
25-Feb-2014 12:29:34 CET Closed file file:run207515_lumi183.root
25-Feb-2014 12:29:34 CET Initiating request to open file file:run207515_lumi184.root
25-Feb-2014 12:29:34 CET Initiating request to open file file:run207515_lumi184.root
25-Feb-2014 12:29:34 CET Successfully opened file file:run207515_lumi184.root
25-Feb-2014 12:29:34 CET Successfully opened file file:run207515_lumi184.root


Reply to this email directly or view it on GitHubhttps://github.com//pull/2581#issuecomment-35998684.

@Dr15Jones
Copy link
Contributor

But since a change in the parameter changes the physics result, albeit slightly, the parameter is required to be tracked.

@VinInn
Copy link
Contributor Author

VinInn commented Feb 25, 2014

as I commented above this to ease testing.

Is untracked exactly not to break configs.
Of course if we go in production with it, it needs to be made tracked as it modifies the products

I was asked to allow more people to test this feature to investigate the difference w.r.t. the full combinatorics and the use of a config param seemed to be the easiest solution.

@slava77
Copy link
Contributor

slava77 commented Feb 25, 2014

For the tracked vs untracked: please change to tracked and use fillDescriptions or existsAs to provide a default value.

@davidlange6
Copy link
Contributor

existsAs should solve the problem without affected anybody, no?.. (long ago we've given up the idea of forcing tracked parameters to be in all configs)

On Feb 25, 2014, at 4:26 PM, Vincenzo Innocente notifications@github.com
wrote:

I think that forcing everybody, in particular HLT to migrate configuration for something that may eventually not go in, is too much a requirement for what I'm doing it.
if untracked is not accepted for formal reasons I will change it to a #define in the code
In any case those investigating this will most probably recompile some part of tracker anyhow.
Please let me know your final decision. @rovere @cerati, if you do not comment further I will move it to a compile time option (off by default, of course)


Reply to this email directly or view it on GitHub.

@VinInn
Copy link
Contributor Author

VinInn commented Feb 25, 2014

On 25 Feb, 2014, at 4:33 PM, davidlange6 notifications@github.com wrote:

existsAs should solve the problem without affected anybody, no?.. (long ago we've given up the idea of forcing tracked parameters to be in all configs)

I will try with existsAs: I have examples…
v.

@VinInn
Copy link
Contributor Author

VinInn commented Feb 25, 2014

wrong default, sorry!

@VinInn
Copy link
Contributor Author

VinInn commented Feb 25, 2014

done. sorry for the time waisted on this point: I was still under the impression that we were forcing tracked parameters to be in all configs.
I take from this that untracked are essentially obsolete.

@slava77
Copy link
Contributor

slava77 commented Feb 25, 2014

Tracked doesn't have to be in the config, this is important for the provenance tracking.
The fact that it's tracked will make it into the provenance (or will be seen as hardcoded, if it is not in provenance).

@cmsbuild
Copy link
Contributor

Pull request #2581 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please check and sign again.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2014

checking #2581 36b4f2e

@rovere @cerati Marco and Giuseppe, please confirm that there are no concerns about these changes from the tracking side.

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2014

@Degano @nclopezo
two days later,

Comparison with the baseline    Still running...

Please fix

[from the code diffs, it's mostly a sanity check]

@cerati
Copy link
Contributor

cerati commented Feb 27, 2014

Yes, we are fine with this changes!

On Feb 27, 2014, at 4:42 PM, slava77 notifications@github.com wrote:

checking #2581 36b4f2e

@rovere @cerati Marco and Giuseppe, please confirm that there are no concerns about these changes from the tracking side.


Reply to this email directly or view it on GitHub.

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2014

+1

#2581 36b4f2e

sanity check: short matrix runs, no diffs

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

@nclopezo
Copy link
Contributor

@slava77
Hi Slava,
I started the comparison again, it is now available:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2581/291/summary.html

nclopezo added a commit that referenced this pull request Feb 28, 2014
RecoLocalTracker -- Fast match
@nclopezo nclopezo merged commit 11c4533 into cms-sw:CMSSW_7_1_X Feb 28, 2014
@nclopezo nclopezo modified the milestones: CMSSW_7_1_0_pre5, CMSSW_7_1_0_pre4 Mar 10, 2014
ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017
@VinInn VinInn deleted the FastMatch branch April 21, 2017 11:47
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