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

Add phase2 Inner Tracker (IT) pixel cluster to cmssw 81X from new IB #14885

Merged

Conversation

emiglior
Copy link
Contributor

@atricomi @ebrondol
This PR superseeds the PR #14854

We introduce a DataFormats + producer for the Inner Tracker pixel clusters for phase2.
The new data format is essentially a clone of the present DataFormats/SiPixelCluster.
The reasons for introducing the new data format are:

  • the current data format for the pixel cluster makes use of types, e.g. https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/DataFormats/SiPixelCluster/interface/SiPixelCluster.h#L77-L87 which optimize the performance with the current detector but do not allow to span the full module in the "small pitch scenario" foreseen for phase2 pixels (e.g. pixel with column or row index larger than 2**8-1=255);
  • more in general, the phase2 specs are still under definition and simulation studies are part of this process. So we would like to be able to change the clusterizer without interfering with the code in use for current/phase1 detector.

The new collection is included in the phase2 workflows but for the moment no rechits are produced out of it.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @emiglior (Ernesto Migliore) for CMSSW_8_1_X.

It involves the following packages:

DataFormats/Phase2ITPixelCluster
RecoLocalTracker/Phase2ITPixelClusterizer
SLHCUpgradeSimulations/Configuration

The following packages do not have a category, yet:

DataFormats/Phase2ITPixelCluster
RecoLocalTracker/Phase2ITPixelClusterizer

@cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @gpetruc, @threus this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@emiglior emiglior changed the title Phase2 it pixel cluster cmssw 81 x from new IB Add phase2 Inner Tracker (IT) pixel cluster to cmssw 81X from new IB Jun 14, 2016
@kpedro88
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 15, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

There are some workflows for which there are errors in the baseline:
5.1 step 2
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

@cvuosalo
Copy link
Contributor

Phase 2 workflow test in progress...

@cvuosalo
Copy link
Contributor

@emiglior: Could you please provide a list of how the C++ header files in this PR are used? That is, where are they used or included? You could post the list here or show it in the Reco meeting on 29 June.
Thanks.

@cvuosalo
Copy link
Contributor

+1

For #14885 030499c

Adding Phase 2 inner tracker pixel clusters. There should be no change in standard workflows.

The new code is marginally acceptable as provisional code that will be re-worked as detector details are finalized, but it will require significant revision to merge it into the mainstream codebase. Jenkins tests against baseline CMSSW_8_1_X_2016-06-27-1100 show no significant differences, as expected. A test of workflow 11024.0_TTBar (2023) with 20 events against baseline CMSSW_8_1_X_2016-06-19-2300 also shows no differences in existing quantities. CPU time increases marginally for the new module (first event excluded from measurement):

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
       added      +0.24%         0.00 ms/ev ->         1.53 ms/ev phase2ITPixelClusters
   -0.274684      -0.06%         1.48 ms/ev ->         1.12 ms/ev siPixelClusters
  ---------- ------------     --------                  ----       ------------
Job total:  0.623225 s/ev ==> 0.662135 s/ev

RECO event size also increases as might be expected:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
      0.0 ->     26202.8      26203     NEWO   7.87     Phase2ITPixelClusteredmNewDetSetVector_phase2ITPixelClusters__RECO.
-------------------------------------------------------------
   332934 ->      359129      26195             7.9     ALL BRANCHES

@civanch
Copy link
Contributor

civanch commented Jun 28, 2016

+1

@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, @Degano, @smuzaffar

@emiglior
Copy link
Contributor Author

Hi, should we assume that the decision about this PR was positive?
Please let us know in order to make a realistic plan for phase2 deliverables in 81X.

@kpedro88
Copy link
Contributor

@slava77 @cvuosalo @davidlange6 let's merge if possible...

@emiglior
Copy link
Contributor Author

Great!

@kpedro88
Copy link
Contributor

kpedro88 commented Jul 3, 2016

@davidlange6 are we going to merge this?

@VinInn
Copy link
Contributor

VinInn commented Jul 4, 2016

Where are we with this PR?
we need it integrated to get the CPE in and start working to use these clusters in OmniCluster, PixelRecHit and reco code

@emiglior
Copy link
Contributor Author

emiglior commented Jul 7, 2016

ping...
We really need this PR in the next pre-release

@kpedro88
Copy link
Contributor

kpedro88 commented Jul 8, 2016

@davidlange6 ...

@davidlange6
Copy link
Contributor

+1
@VinInn and I discussed some options forward, but will approve this for now so that next steps happen in parallel. I the meanwhile, undoubtedly this will trigger much more code copying downstream that hopefully can be avoided with some thought by the development team.

@cmsbuild cmsbuild merged commit 6b27034 into cms-sw:CMSSW_8_1_X Jul 20, 2016
@emiglior
Copy link
Contributor Author

Thx to all the involved!
Does it mean that will be included in 810_pre9?

@kpedro88
Copy link
Contributor

@emiglior yes, pre9 has not been built yet, so all PRs merged so far will be included.

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

10 participants