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

bugfix for pixel clustershape filter, and, change HI tracking low-pt seeding from triplet to track #6376

Merged
merged 2 commits into from Nov 14, 2014

Conversation

yetkinyilmaz
Copy link
Contributor

As listed in the commits, first commit is for bugfixing the clustershape filter, removing conflict between two vs three argument version of the operator(). This was a prerequisite for the update of low-pt tracking.

In the second commit, the HI tracking uses tracks instead of triplets in seeding, which is a significant improvement in timing. Details in: https://indico.cern.ch/event/345510/contribution/5/material/slides/0.pdf

This PR requires #6374 to be merged, in order to run the full HI reco sequence successfully.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @yetkinyilmaz for CMSSW_7_3_X.

bugfix for pixel clustershape filter, and, change HI tracking low-pt seeding from triplet to track

It involves the following packages:

RecoHI/HiTracking
RecoPixelVertexing/PixelTrackFitting

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@echapon, @makortel, @GiacomoSguazzoni, @jazzitup, @VinInn, @appeltel, @richard-cms, @yenjie, @rovere, @kurtejung, @cerati, @mandrenguyen, @dgulhan 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 you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@@ -6,7 +6,7 @@ namespace edm { class Event; class EventSetup; class ConsumesCollector;}
class TrackingRecHit;

#include <vector>

#include "DataFormats/TrackerCommon/interface/TrackerTopology.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe forward declare it?

@slava77
Copy link
Contributor

slava77 commented Nov 13, 2014

@cerati @rovere @VinInn
The change looks simple, still, please check and say if it's OK.
Thanks.

@rovere
Copy link
Contributor

rovere commented Nov 13, 2014

Ciao Slava,
looks ok to me.

M.

@@ -56,6 +56,7 @@ PixelTrackReconstruction::PixelTrackReconstruction(const ParameterSet& cfg,
if(theConfig.exists("useFilterWithES")) {
edm::LogInfo("Obsolete") << "useFilterWithES parameter is obsolete and can be removed";
}
useClusterShape = filterPSet.exists("useClusterShape");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also the value of useClusterShape parameter be checked (in addition of its existence)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is just tells you whether the class inherits from
clusterShapeTrackFilter or not.
If it does, it should use the 3 parameter version of the operator(),
whether or not the parameter is set to True.
There is certainly a more elegant solution, but this was the minimally
invasive approach.

On 11/13/14 3:52 PM, Matti Kortelainen wrote:

In RecoPixelVertexing/PixelTrackFitting/src/PixelTrackReconstruction.cc:

@@ -56,6 +56,7 @@ PixelTrackReconstruction::PixelTrackReconstruction(const ParameterSet& cfg,
if(theConfig.exists("useFilterWithES")) {
edm::LogInfo("Obsolete") << "useFilterWithES parameter is obsolete and can be removed";
}

  • useClusterShape = filterPSet.exists("useClusterShape");

Should also the value of useClusterShape parameter be checked (in
addition of its existence)?


Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/6376/files#r20295348.


Matthew Nguyen
Chargé de Recherche
LLR-Ecole Polytechnique
91128 Palaiseau FRANCE

+33 1 69 33 55 65

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, and the filter itself checks the value. Got it, thanks.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 13, 2014

testing ... together with #6374.
It would've been more practical in this case to have #6376 to include #6374 , since #6374 is fairly trivial.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 14, 2014

+1

for #6376 f7013cc
CMSSW_7_3_X_2014-11-13-1400 (test area sign461)

Here is a brief summary of what I see from a combination of #6374 and this PR on top of CMSSW_7_3_X_2014-11-13-1400.

In general, the changes are in the expected direction:
(the plots are all based on 140.53, 400 events, 182124:40 from UPC)

  • initialStep tracks now show up
    all_sign461vsorig_runhi2011wf140p53c_recotracks_higeneraltracks__rereco_obj_algo
  • more tracks in the central
    all_sign461vsorig_runhi2011wf140p53c_recotracks_higeneraltracks__rereco_obj_eta
  • the number of muons increases, but most of them are probably fakes
    • the rate of global muons is about the same
      the trkKink plotted points to more tracks with inflections along the trajectory
      all_sign461vsorig_runhi2011wf140p53c_recomuons_muons__rereco_obj_combinedquality_trkkink
  • large increase in particleFlowEGamma in the barrel
    all_sign461vsorig_runhi2011wf140p53c_recopfcandidates_particleflowegamma__rereco_obj_eta
    all_sign461vsorig_runhi2011wf140p53c_recopfcandidates_particleflowegamma__rereco_obj_pt
  • most of these changes in upstream objects are not making it through as visible post-PF changes
    all_sign461vsorig_runhi2011wf140p53c_log10recopfjets_akvs4pfjets__rereco_obj_et

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

cmsbuild added a commit that referenced this pull request Nov 14, 2014
bugfix for pixel clustershape filter, and, change HI tracking low-pt seeding from triplet to track
@cmsbuild cmsbuild merged commit 4570ab0 into cms-sw:CMSSW_7_3_X Nov 14, 2014
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