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

Strip cluster shape filter with subcluster info #5583

Merged

Conversation

gpetruc
Copy link
Contributor

@gpetruc gpetruc commented Sep 26, 2014

Introduce a new strip cluster shape filter:

  • in the trajectory building of the initial, pixelPair and detachedTriplet steps, only for the TIB and TID layers 1,2
  • in the seed building for pixelLess (replacing the old one) and tobTec (where there was none)

Presented at the tracking POG meeting on 22 Sep 2014, https://indico.cern.ch/event/337840/
After the meeting, it was verified that it doesn't have ill effects on the QCD 3TeV relval, and that the small inefficiency introduced in the pixelLess and tobTec does not depend on the transverse impact parameter of the tracks.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @gpetruc (Giovanni Petrucciani) for CMSSW_7_3_X.

Strip cluster shape filter with subcluster info

It involves the following packages:

RecoPixelVertexing/PixelLowPtUtilities
RecoTracker/IterativeTracking
RecoTracker/MeasurementDet

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @GiacomoSguazzoni, @rovere, @VinInn, @cerati, @venturia 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.

…,detachedTriplet). Switching on SeedFilter for pixelLess, tobTec
@gpetruc gpetruc force-pushed the simpleStripSubClusterFilter-from72X branch from 9464288 to af5162f Compare September 26, 2014 12:29
@gpetruc
Copy link
Contributor Author

gpetruc commented Sep 26, 2014

sorry for the extra commit, I had realized that I had forgotten to git add a fix to a typo in one of the python files.

@cmsbuild
Copy link
Contributor

Pull request #5583 was updated. @cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please check and sign again.

const TrackingRecHit* hit = thit->hit();
if (hit == 0 || !hit->isValid()) return true;
if (hit->geographicalId().subdetId() <= 2) return true; // we look only at strips for now
return testLastHit(hit, tsos, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

The code in this function is mostly repeated four times (in this file). Can't you template it or anyway modularize it better?

@cerati cerati mentioned this pull request Sep 26, 2014
const StripGeomDetUnit* stripDetUnit = dynamic_cast<const StripGeomDetUnit *>(det);
if (det == 0) { edm::LogError("Strip not a StripGeomDetUnit?") << " on " << detId.rawId() << "\n"; return true; }

float MeVperADCStrip = 3.61e-06*265;
Copy link
Contributor

Choose a reason for hiding this comment

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

Another small point - leave a comment here about the intention; no idea why the 265 - I suppose it has nothing to do with the other "magic" number used (twice!) above, namely the 254. Generally - make constants, don't use numbers inside logical expressions, leave a comment.

@VinInn
Copy link
Contributor

VinInn commented Sep 29, 2014

@cmsbuild tests? comparison?

@cmsbuild
Copy link
Contributor

@StoyanStoynev
Copy link
Contributor

I am showing some results below based on wf 202 (ttbar + PU) and wf 38 (flat pt QCD). I checked other wf too but generally there is nothing to report from them. An exception seems to be HI RECO - it fails in wf 140.51 (RunHI2010), I should check with others HI wf too before investigating:
...
File "/build/stoyan/tests/pull5583/CMSSW_7_3_X_2014-09-24-1400/python/RecoHI/HiMuonAlgos/HiRegitMuonInitialStep_cff.py", line 55, in
hiRegitMuInitialStepTrajectoryFilter.minPt = 2.5 # after each new hit, apply pT cut for traj w/ at least minHitsMinPt = cms.int32(3),
...
raise TypeError(name+" does not already exist, so it can only be set to a CMS python configuration type")
TypeError: minPt does not already exist, so it can only be set to a CMS python configuration type

Back to wf 202, overall significant changes to seed multiplicity in iterations 5 and 6 (mostly), average layers/hits per tracks go slightly down, fake rate goes down. From higher level objects only b-tagging performance show some deviation (depending on the discriminant). This PR is in red below (black is CMSSW_7_3_X_2014-09-24-1400)

wf202_track_seedeta_iter4

wf202_track_seedeta_iter5

wf202_track_seedeta_iter6

wf202_track_tib_rhpertrk_vs_eta

wf202_track_validhitspertrack_vs_eta

wf203_track_meanlayerspertrack

wf204_track_fakerate_vs_eta

Track efficiency vs vertex position changes in tails (this is wf 38.0):

wf38_track_eff_vs_vertpos

wf38_track_eff_vs_dxy

B-tagging (from wf 202.0):

wf202_btag_jbp_cdisc

wf202_btag_jbp_dusg

wf202_btag_tche_dusg

@cmsbuild
Copy link
Contributor

@StoyanStoynev
Copy link
Contributor

+1
Based on the review above and the additional tests here.
Tested 3b23ce6 vs af5162f (the previous version discussed above) - both on top of CMSSW_7_3_X_2014-09-24-1400.
There are quite a lot of numerical differences (short matrix + other ttbar samples; FWlite + DQM scripts ) like this, wf 202:
all_some_pull5583_v2__vs__pull5583_ttbarpuwf202p0c_recotracks_generaltracks__reco_obj_eta
affecting mostly jets:
all_some_pull5583_v2__vs__pull5583_ttbarpuwf202p0c_recopfjets_ak8pfjets__reco_obj_eta
Those are fine, no other issues found. HI RECO is "cured".

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2014

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). @nclopezo, @ktf can you please take care of it?

@Dr15Jones
Copy link
Contributor

The static analyzer run on this pull request shows a number of problems

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5583/111/llvm-analysis/

@StoyanStoynev
Copy link
Contributor

@Dr15Jones What is included in the reports - all the dependent sources? Many of the files reported are not touched at all in this PR.

@Dr15Jones
Copy link
Contributor

Yes, everything that had to be recompiled gets scanned by the static analyzer (since the static analyzer works just like a compiler).

@StoyanStoynev
Copy link
Contributor

As far as I see none of the reports is related to this PR.
It would be easier if the report manages to at least emphasise the files that were changed in the PR
(that is the match between files in the file lists from the Static Analyzer and the PR)

davidlange6 added a commit that referenced this pull request Oct 2, 2014
…m72X

Strip cluster shape filter with subcluster info
@davidlange6 davidlange6 merged commit 6c9bef7 into cms-sw:CMSSW_7_3_X Oct 2, 2014
@gpetruc gpetruc deleted the simpleStripSubClusterFilter-from72X branch June 1, 2016 07:48
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

8 participants