Improvements in track quality criteria in PF for muon seeded iterations (76X) #11434

bachtis · 2015-09-23T14:28:15Z

slava77 · 2015-09-23T14:31:00Z

RecoParticleFlow/PFProducer/plugins/importers/ImprovedTracksImporter.cc

+		     << std::endl;
+    if (_debug) std::cout << " cut is DPt/Pt < " << _DPtovPtCut[Algo] * sigmaHad << std::endl;
+    if (_debug) std::cout << " cut is NHit >= " << _NHitCut[Algo] << std::endl;
+    /*


please cleanup commented out parts ... if needed keep behind LogTrace or LogDebug (preferred)

cmsbuild · 2015-09-23T14:31:12Z

A new Pull Request was created by @bachtis (Michalis Bachtis) for CMSSW_7_6_X.

Improvements in track quality criteria in PF for muon seeded iterations (76X)

It involves the following packages:

RecoParticleFlow/PFProducer

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @lgray 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

slava77 · 2015-09-23T14:32:58Z

RecoParticleFlow/PFProducer/plugins/importers/ImprovedTracksImporter.cc

+#include "DataFormats/MuonReco/interface/Muon.h"
+#include "RecoParticleFlow/PFProducer/interface/PFMuonAlgo.h"
+
+class ImprovedTracksImporter : public BlockElementImporterBase {


"Improved" is generally not a good name
What will happen to the next developments? "ImprovedImproved"?

Clearly, "BetterImproved", followed by "UpgradedBetterImproved"...

lgray · 2015-09-23T14:37:45Z

@bachtis Not possible to extend the existing general tracks importer? (to avoid these codes from diverging over time?)

bachtis · 2015-09-23T14:40:45Z

@lgray No because then HLT cfg needs reparsing Read the description before you make comments
@lgray , @slava77 I like the name and I couldnt think of a better one

lgray · 2015-09-23T14:54:59Z

@bachtis I read your description, that says it is inconvenient rather than impossible, which is why I made the comment.

It is not good to have two sets of code that do mostly the same thing. Since it is going to 75X there is likely time to reparse the HLT configs.

However, if time is pressing. This can be taken care of in another PR.

@slava77 @Martin-Grunewald comments?

lgray · 2015-09-23T14:59:49Z

Also, what about GeneralTracksImporterV2? Much less wordy and has the benefit of not needing a countably infinite list of adjectives.

bachtis · 2015-09-23T15:00:55Z

Well if @Martin-Grunewald agrees with reparsing we can replace the old one. The matrix tests will crash at HLT if this is not done though .

lgray · 2015-09-23T15:02:39Z

@bachtis Make a customization function named customizeFor_(PRNumber) and stick it in HLTrigger/Configuration. There are a few examples in there. This will allow matrix tests to run.

bachtis · 2015-09-23T15:04:16Z

OK then lets see if it is OK to change the hltParticleFlowBlock module.and then decide how to proceed

davidlange6 · 2015-09-23T15:06:14Z

the hlt is supporting changes to 75x via customize functions. Should be easy

On Sep 23, 2015, at 5:00 PM, Michalis Bachtis notifications@github.com wrote:

Well if @Martin-Grunewald agrees with reparsing we can replace the old one. The matrix tests will crash at HLT if this is not done though .

—
Reply to this email directly or view it on GitHub.

bachtis · 2015-09-23T15:07:31Z

so can somebody tell me what exactly I have to do- which files to change? In that case I would prefer to modify the old importer instead of adding new one

Martin-Grunewald · 2015-09-23T15:23:19Z

If you'd use a fillDescriptions method, you can add all the parameters you want,
without HLT parsing, if the default values are fine for HLT. No need for customisation.

Martin-Grunewald · 2015-09-23T15:26:14Z

@davidlange6
Why are we allowing new code in without fillDescriptions?
(Cf ORP wiki blurb?)

davidlange6 · 2015-09-23T15:28:31Z

An even better solution.

On Sep 23, 2015, at 5:26 PM, Martin Grunewald <notifications@github.com mailto:notifications@github.com> wrote:

@davidlange6 https://github.com/davidlange6
Why are we allowing new code in without fillDescriptions?
(Cf ORP wiki blurb?)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11434#issuecomment-142639079.

lgray · 2015-09-23T15:31:33Z

The python change for this happens in some PSet inside of a VPSet. Is there a way to do fillDescriptions for an individual plugin in a list?

bachtis · 2015-09-23T15:50:23Z

HI . I am running the matrix. In fact since HLT iterations are different it might work out of the box .

bachtis · 2015-09-23T16:10:04Z

In fact no intervention is needed in HLT config . Implemented code review comments. PR should be updated now

cmsbuild · 2015-09-23T16:11:15Z

Pull request #11434 was updated. @cmsbuild, @cvuosalo, @slava77 can you please check and sign again.

Dr15Jones · 2015-09-23T16:13:11Z

Yes one can have a fillDescription state the properties of PSets within a VPSet.

slava77 · 2015-09-23T16:25:54Z

RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporter.cc

@@ -33,6 +38,9 @@ class GeneralTracksImporter : public BlockElementImporterBase {
  const std::vector<double> _DPtovPtCut;
  const std::vector<unsigned> _NHitCut;
  const bool _useIterTracking,_cleanBadConvBrems,_debug;
+
+  PFMuonAlgo *pfmu_;


unique_ptr indeed, please.

slava77 · 2015-09-23T18:03:00Z

@cmsbuild please test
[I suppose migration to fillDescriptions is not happening, since now it's an "old code" fix]

lgray · 2015-09-23T18:03:23Z

@slava77 already asked for test..

cmsbuild · 2015-09-23T19:57:21Z

+1
Tested at: bc37c82
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11434/8303/summary.html

cmsbuild · 2015-09-23T20:57:56Z

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

slava77 · 2015-09-28T16:03:05Z

I'm running higher stat tests to see the effects.
Code looks ok (no changes needed from visual inspection)

slava77 · 2015-09-29T00:00:43Z

+1

for #11434 bc37c82

track cleaning updates: expect less charged hadrons
jenkins tests pass and comparisons with baseline show no differences (the phase space of changes is not sampled in the tests; it is small)
tested locally in CMSSW_7_6_X_2015-09-22-1500 : changes are observed in higher stat tests with high-pt tracks occurring naturally: in wf 1313 (3 TeV dijet) and muon 1 TeV gun
- changes in wf1313 are more significant and are in a more natural environment:
  - 22/400 (~5%) events have differences
  - about a half of events have larger MET ( 4 larger by 20%; 3 events have MET going smaller by 20%). So, relative to MET in this sample the change isn't making an improvement.
  - of all PFCandidates in the event: there are fewer high-pt charged hadrons (-22) and muons (-1) and more photons (+14) and neutral hadrons (+13). This migration is expected if a large energy is present.
- muon 1 TeV sample doesn't have particularly large changes
==> things look OK in the "normal" events. For the signoff I'm relying more on observations from the more rare events cases used to make this PR.

cmsbuild · 2015-09-29T00:01:09Z

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

davidlange6 · 2015-09-29T05:42:05Z

+1

Improvements in track quality criteria in PF for muon seeded iterations (76X)

lgray · 2015-09-29T09:21:40Z

@bachtis Does the migration observed in wf1313 have any sizeable impact on the resolution of high pT jets?

lgray · 2015-09-29T09:45:53Z

@bachtis Just asking since there are no plots on the PR...

bachtis · 2015-09-29T10:13:15Z

Should not have any impact . If you have a ~TeV track that will be killed the calorimeter should measure it better . Only one event changed in the Flat QCD sample I rerecoed so I dont expect any effect

slava77 · 2015-09-29T10:24:18Z

I could rerun on more events. There wasn't much to see with 22 events
changing only.
On Sep 29, 2015 5:13 AM, "Michalis Bachtis" notifications@github.com
wrote:

Should not have any impact . If you have a ~TeV track that will be killed
the calorimeter should measure it better . Only one event changed in the
Flat QCD sample I rerecoed so I dont expect any effect

—
Reply to this email directly or view it on GitHub
#11434 (comment).

lgray · 2015-09-29T10:27:21Z

@bachtis Sure, that I can believe, but need the plot. My being curious is simply healthy skepticism, particularly given the wf1313 changes @slava77 mentions upstairs.

@slava77 If we could put a plot here it would help. (I guess 1k events or something?)

slava77 · 2015-09-29T12:48:53Z

@lgray I don't think 1K events will be enough (4K maybe).
Here are a couple of plots from the tests done already (from wf 1313)

First is to show that tracks go away on the high end of chFrac (this is a more clear "thing are in the right direction" plot)
$wf1313_ak4pf_chfrac$

And the reco/gen plot for high et jets: it's not very convincingly mildly better

lgray · 2015-09-29T12:50:52Z

@slava77 @bachtis Ahh, OK this is certainly in the noise concerning the resolution. Thanks.

Track Quality Improvements

6ce18cc

slava77 reviewed Sep 23, 2015
View reviewed changes

cmsbuild added this to the Next CMSSW_7_6_X milestone Sep 23, 2015

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Sep 23, 2015

slava77 reviewed Sep 23, 2015
View reviewed changes

Well in fact new IMporter is not needed

5bdffba

Removing commented lines

5693652

slava77 reviewed Sep 23, 2015
View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Sep 23, 2015

cmsbuild added tests-approved and removed tests-started labels Sep 23, 2015

cmsbuild added comparison-available and removed comparison-pending labels Sep 23, 2015

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Sep 29, 2015

slava77 mentioned this pull request Sep 29, 2015

Improvements in track quality criteria in PF for muon seeded iterations #11433

Merged

cmsbuild added orp-approved and removed orp-pending labels Sep 29, 2015

cmsbuild added a commit that referenced this pull request Sep 29, 2015

Merge pull request #11434 from bachtis/PFTrackQuality

85bae3c

Improvements in track quality criteria in PF for muon seeded iterations (76X)

cmsbuild merged commit 85bae3c into cms-sw:CMSSW_7_6_X Sep 29, 2015

Improvements in track quality criteria in PF for muon seeded iterations (76X) #11434

Improvements in track quality criteria in PF for muon seeded iterations (76X) #11434

Conversation

bachtis commented Sep 23, 2015

slava77 Sep 23, 2015

Choose a reason for hiding this comment

cmsbuild commented Sep 23, 2015

slava77 Sep 23, 2015

Choose a reason for hiding this comment

lgray Sep 23, 2015

Choose a reason for hiding this comment

lgray commented Sep 23, 2015

bachtis commented Sep 23, 2015

lgray commented Sep 23, 2015

lgray commented Sep 23, 2015

bachtis commented Sep 23, 2015

lgray commented Sep 23, 2015

bachtis commented Sep 23, 2015

davidlange6 commented Sep 23, 2015

bachtis commented Sep 23, 2015

Martin-Grunewald commented Sep 23, 2015

Martin-Grunewald commented Sep 23, 2015

davidlange6 commented Sep 23, 2015

lgray commented Sep 23, 2015

bachtis commented Sep 23, 2015

bachtis commented Sep 23, 2015

cmsbuild commented Sep 23, 2015

Dr15Jones commented Sep 23, 2015

slava77 Sep 23, 2015

Choose a reason for hiding this comment

slava77 commented Sep 23, 2015

lgray commented Sep 23, 2015

cmsbuild commented Sep 23, 2015

cmsbuild commented Sep 23, 2015

slava77 commented Sep 28, 2015

slava77 commented Sep 29, 2015

cmsbuild commented Sep 29, 2015

davidlange6 commented Sep 29, 2015

lgray commented Sep 29, 2015

lgray commented Sep 29, 2015

bachtis commented Sep 29, 2015

slava77 commented Sep 29, 2015

lgray commented Sep 29, 2015

slava77 commented Sep 29, 2015

lgray commented Sep 29, 2015