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

Reduce threshold for tracks in vertexers used for timing studies #15989

Merged

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Sep 26, 2016

Improves efficiency for vertex finding (1 GeV cut was there for testing purposes, easier to understand).

@lgray
Copy link
Contributor Author

lgray commented Sep 26, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 26, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15381/console

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_8_1_X.

It involves the following packages:

RecoVertex/Configuration

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @dgulhan, @rovere, @VinInn this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • 10424.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017NewFPix_GenSimFull+DigiFull_2017NewFPix+RecoFull_2017NewFPix+HARVESTFull_2017NewFPix
  • 10624.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017HCALdev_GenSimFull+DigiFull_2017HCALdev+RecoFull_2017HCALdev+HARVESTFull_2017HCALdev
  • 10824.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017AllNew_GenSimFull+DigiFull_2017AllNew+RecoFull_2017AllNew+HARVESTFull_2017AllNew

@cvuosalo
Copy link
Contributor

cvuosalo commented Sep 27, 2016

+1

For #15989 ca49f46

For Phase 2, reducing the minimum pT threshold for tracks in vertexers used for timing studies.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-09-26-1500 show no significant differences. A test of workflow 22424.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D3Timing with 100 events against baseline CMSSW_8_1_0_pre12 also shows no significant differences, just small fluctuations related to the additional primary vertices. RECO event content shows the effect of reducing the minimum pT, thereby increasing the size of primary vertex collections:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
    317.0 ->       371.9         55     15.9   0.00     recoVertexs_offlinePrimaryVertices1DWithBS__RECO.
    279.1 ->       320.1         41     13.7   0.00     recoVertexs_offlinePrimaryVertices4DWithBS__RECO.
    320.3 ->       377.6         57     16.4   0.00     recoVertexs_offlinePrimaryVertices1D__RECO.
    288.0 ->       329.3         41     13.4   0.00     recoVertexs_offlinePrimaryVertices4D__RECO.
-------------------------------------------------------------
  3601978 ->     3602158        181             0.0     ALL BRANCHES

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Sep 27, 2016

hold

On 9/27/16 1:43 PM, Carl Vuosalo wrote:

+1

For #15989 #15989 ca49f46
ca49f46

For Phase 2, reducing the minimum pT threshold for tracks in vertexers
used for timing studies.

The code changes are satisfactory, and Jenkins tests against baseline
CMSSW_8_1_X_2016-09-26-1500 show no significant differences. Jenkins
workflow 20024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D1 shows numerous
exceedingly tiny differences caused by the primary vertex changes.

I see that the post on github is edited and does not mention changes any
more

Jenkins tests have differences in TTbar13TeV2023D1wf20024p0
We need to understand why.

A
test of workflow 22424.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D3Timing
with 100 events against baseline CMSSW_8_1_0_pre12 also shows no
significant differences, just small fluctuations related to the
additional primary vertices. RECO event content shows the effect of
reducing the minimum pT, thereby increasing the size of primary vertex
collections:

|----------------------------------------------------------------- or, B
new, B delta, B delta, % deltaJ, % branch
----------------------------------------------------------------- 317.0
-> 371.9 55 15.9 0.00 recoVertexs_offlinePrimaryVertices1DWithBS__RECO.
279.1 -> 320.1 41 13.7 0.00
recoVertexs_offlinePrimaryVertices4DWithBS__RECO. 320.3 -> 377.6 57 16.4
0.00 recoVertexs_offlinePrimaryVertices1D__RECO. 288.0 -> 329.3 41 13.4
0.00 recoVertexs_offlinePrimaryVertices4D__RECO.
------------------------------------------------------------- 3601978 ->
3602158 181 0.0 ALL BRANCHES |


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15989 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbpsfHxlWTnuB4ZXIC2fswSju47YIks5quX_agaJpZM4KGvuK.

@cmsbuild
Copy link
Contributor

Pull request has been put on hold by @slava77
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cvuosalo
Copy link
Contributor

@slava77: The same differences showed up in #15907, so they are not related to this PR. I figured they were a glitch in some of the IBs.

@slava77
Copy link
Contributor

slava77 commented Sep 27, 2016

On 9/27/16 2:36 PM, Carl Vuosalo wrote:

@slava77 https://github.com/slava77: The same differences showed up in
#15907 #15907, so they are not
related to this PR. I figured they were a glitch in some of the IBs.

Please check if this can be reproduced in local tests.

We have to follow up on reco non-reproducibility cases.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15989 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbjpbeCNRc7WwLCk3lOzJbXGTkCyQks5quYxkgaJpZM4KGvuK.

@lgray
Copy link
Contributor Author

lgray commented Sep 27, 2016

@slava77 this PR doesn't touch any C++ and only touches vertexer producers
specific to timing era. Change to D1 vertexing without timing cannot be
from this PR.

On Tue, Sep 27, 2016 at 4:41 PM, Slava Krutelyov notifications@github.com
wrote:

On 9/27/16 2:36 PM, Carl Vuosalo wrote:

@slava77 https://github.com/slava77: The same differences showed up in
#15907 #15907, so they are not
related to this PR. I figured they were a glitch in some of the IBs.

Please check if this can be reproduced in local tests.

We have to follow up on reco non-reproducibility cases.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15989 (comment), or
mute the thread
<https://github.com/notifications/unsubscribe-auth/
AEdcbjpbeCNRc7WwLCk3lOzJbXGTkCyQks5quYxkgaJpZM4KGvuK>.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15989 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABBMOfpuQUXYkroD2eBiRgb5h2V4rB67ks5quY1ugaJpZM4KGvuK
.

@slava77
Copy link
Contributor

slava77 commented Sep 27, 2016

unhold

@cvuosalo please open an issue on github related to the reproducibility
in 20024
so that it is not lost.
It better be understood before we close pre13.
My guess for a "more likely" origin is in the SIM or digi.

On 9/27/16 2:46 PM, Lindsey Gray wrote:

@slava77 this PR doesn't touch any C++ and only touches vertexer producers
specific to timing era. Change to D1 vertexing without timing cannot be
from this PR.

The more clear evidence that it is unrelated is that the same issue
happens in another PR that has no tracking changes.

On Tue, Sep 27, 2016 at 4:41 PM, Slava Krutelyov notifications@github.com
wrote:

On 9/27/16 2:36 PM, Carl Vuosalo wrote:

@slava77 https://github.com/slava77: The same differences showed up in
#15907 #15907, so they are not
related to this PR. I figured they were a glitch in some of the IBs.

Please check if this can be reproduced in local tests.

We have to follow up on reco non-reproducibility cases.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15989 (comment), or
mute the thread
<https://github.com/notifications/unsubscribe-auth/
AEdcbjpbeCNRc7WwLCk3lOzJbXGTkCyQks5quYxkgaJpZM4KGvuK>.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15989 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABBMOfpuQUXYkroD2eBiRgb5h2V4rB67ks5quY1ugaJpZM4KGvuK
.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15989 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbsuj5XwtxNjj96u4nmtKKYk7MvpXks5quY6rgaJpZM4KGvuK.

@cmsbuild
Copy link
Contributor

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

@cvuosalo
Copy link
Contributor

@slava77: Issue #16004 has been opened.

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b4c749d into cms-sw:CMSSW_8_1_X Sep 28, 2016
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

5 participants