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

Cluster cleaning bug fix, renamed hiSecond to hiLowPt #7686

Merged

Conversation

mandrenguyen
Copy link
Contributor

One line bugfix to cluster cleaning between before low pT step of HI tracking sequence, introduced by recent addition of a detached triplet step.
The line:
oldClusterRemovalInfo = cms.InputTag("hiDetachedTripletStepClusters")
was missing from PR #7393
The effect is that only clusters from the step immediately preceding the low pT step were removed (but not those from the intial step), leading to duplication of work.

We take advantage of the opportunity to change 'hiSecondPixelTriplet' to 'hiLowPtPixelTriplet', as the low pT step is no longer second (as promised in the comments of #7393). This follows the style of the pp code.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mandrenguyen for CMSSW_7_4_X.

Cluster cleaning bug fix, renamed hiSecond to hiLowPt

It involves the following packages:

RecoHI/HiTracking

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@echapon, @jazzitup, @dgulhan, @appeltel, @yenjie, @kurtejung, @richard-cms, @yetkinyilmaz 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
Copy link
Contributor

slava77 commented Feb 11, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #7686 be926f4

This PR is described as a small bug fix to reduce duplication of work in the HI tracking sequence and a simple renaming for clarity. It turns out to cause numerous small changes in the distributions of quantities related to HI tracks, as will be illustrated below.

The code change is satisfactory, and most the Jenkins tests are OK. But it appears that Jenkins somehow included some other PRs in its tests, so several spurious differences appear in the results in the alternative-comparisons and validateJR.

Matrix tests were run against baseline CMSSW_7_4_X_2015-02-12-0200, and they show no significant differences except for workflow 140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI. An extended test for 100 events was run for 140.53 to obtain higher statistics.

In terms of technical performance of the new code, the extended test results show a small decrease of 1-2% in processing time per event, a small 0.4% decrease in total product size, and a small decrease of about 2% in memory usage.

Many differences in reconstruction-related distributions show up for 140.53. Some of the larger ones are below. The red is new, the black is the reference.

all_testpr7686vsorig_runhi2011wf140p53c_recotracks_higeneraltracks__rereco_obj_algo

all_testpr7686vsorig_runhi2011wf140p53c_recotracks_higeneraltracks__rereco_obj_found

all_testpr7686vsorig_runhi2011wf140p53c_recotracks_higeneraltracks__rereco_obj_ndof

all_testpr7686vsorig_runhi2011wf140p53c_log10recopfjets_akvs4pfjets__rereco_obj_et

algotrk

dcastats

hitsptrkhi

layerptrkalca

layerptrkhi

trkqual

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

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.

6 participants