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

Pixel cluster splitting and jet core iteration for HI Tracking #10775

Merged
merged 12 commits into from Aug 26, 2015

Conversation

dgulhan
Copy link
Contributor

@dgulhan dgulhan commented Aug 14, 2015

Pixel cluster splitting and jet core iteration as it is in done in pp tracking is implemented in HI tracking. The difference is the jet collection, jets used in heavy ions have iterative Pu background subtraction.

Latest related presentations in HI Tracking meetings:
https://indico.cern.ch/event/438299/

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @dgulhan for CMSSW_7_6_X.

Pixel cluster splitting and jet core iteration for HI Tracking

It involves the following packages:

Configuration/StandardSequences
RecoHI/Configuration
RecoHI/HiJetAlgos
RecoHI/HiTracking
RecoLocalTracker/Configuration

@cmsbuild, @cvuosalo, @franzoni, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @yslai, @forthommel, @yduhm, @gbenelli, @echapon, @threus, @makortel, @jlagram, @cerati, @mandrenguyen, @yetkinyilmaz, @GiacomoSguazzoni, @rovere, @VinInn, @richard-cms, @nickmccoll, @MiheeJo, @jazzitup, @yenjie, @Martin-Grunewald, @kurtejung, @gpetruc, @istaslis 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 Aug 14, 2015

@dgulhan please cleanup commented out code (remove it or give a good explanation why it's still needed there, e.g., new developments coming imminently will need)

@cmsbuild
Copy link
Contributor

Pull request #10775 was updated. @cmsbuild, @cvuosalo, @franzoni, @davidlange6, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2015

I see that some functional changes are still being done to this PR.
@dgulhan please post a clear message when you are done (also, please take care of #10775 (comment))
we will not start testing until then

If there's work in progress, it makes more sense to do it locally and git push only in the end after everything looks good

@dgulhan
Copy link
Contributor Author

dgulhan commented Aug 14, 2015

Commented out code was cleaned up. Apologies for the change in the parameters, it was intentional. The current parameters were tested, and the earlier version was a mistake. We are done with our changes now.

@cmsbuild
Copy link
Contributor

Pull request #10775 was updated. @cmsbuild, @cvuosalo, @franzoni, @davidlange6, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2015

@cmsbuild please test

@dgulhan thank you for the prompt cleanup and the message

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-1
Tested at: ef23f25
When I ran the RelVals I found an error in the following worklfows:
4.22 step1

DAS Error

4.53 step1

DAS Error

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10775/7334/summary.html

@cvuosalo
Copy link
Contributor

@cmsbuild please test
Try again, hoping for no DAS errors

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@dgulhan
Copy link
Contributor Author

dgulhan commented Aug 17, 2015

The timing study as a function of number of tracks is shown here:
https://indico.cern.ch/event/438299/contribution/5/attachments/1139921/1632502/clustersplitting_04.pdf
It was done for a dijet 460 embedding, so all of the events have at least one jet that pass the 100 GeV requirement for the hiJetCoreRegionalStepSeeds to run. It was shown that the timing is negligible compared to rest of the iterations in HI track reconstruction as a function of multiplicity.

As an additional safety, a filter is added on number of jets (highest pT 4 jets) to avoid large timing for events with many jets.
dgulhan@bcd1898
dgulhan@05ab60e
dgulhan@bb138f3
dgulhan@ef62943

Such events were found to occur in data in the past, due to beam scraping events happening simultaneously with collisions events. Such events are not found in MC.

@cmsbuild
Copy link
Contributor

Pull request #10775 was updated. @cmsbuild, @cvuosalo, @franzoni, @davidlange6, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Aug 17, 2015

@dgulhan could you please give a pointer to the files that can be tested using this PR directly.

@slava77
Copy link
Contributor

slava77 commented Aug 18, 2015

I modified matrix workflow 301.0 to make HIMIXed events with pthat 480 to 520 GeV and made 100 events

This gives some events that would pass the jet core.

While not very obvious from the PR description " jet core iteration as it is in done in pp tracking", the seeding is very different from the pp case: pixel triplets or pixdoublet+TIB are used for seeding, compared to just doublets in pp. This should reduce the combinatorial growth by quite a bit and make the algorithm fairly manageable even in high hit density environment.

only in the new (>=10 ms/evt):
        hiPixelAdaptiveVertexPreSplitting        13.4137 ms/ev
        hiAkPu4CaloJetsForTrkPreSplitting        144.611 ms/ev
        siPixelClusters          83.3496 ms/ev
        siPixelRecHits   38.4431 ms/ev
        siPixelClusterShapeCache         21.6268 ms/ev
        hiPixel3ProtoTracks      31.9807 ms/ev
        akPu4CaloJetsForTrk      145.853 ms/ev
        hiJetCoreRegionalStepSeeds       62.1432 ms/ev
        hiJetCoreRegionalStepTrackCandidates     1481.61 ms/ev
... here 1 event took 55 s/evt
        hiJetCoreRegionalStepTracks      27.1986 ms/ev
        hiClusterCompatibility   115.968 ms/ev
         Total only f2: 2172.66 ms/evt

@cvuosalo
Carl, I think we can sign off

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #10775 ef62943

Second approval by Reco. The latest code changes are satisfactory, and performance test results (above) show acceptable performance. New Jenkins tests still show no significant differences.

@slava77
Copy link
Contributor

slava77 commented Aug 24, 2015

@franzoni @davidlange6 [for operations-pending]
It has been 6 days already.
Are there any issues with this PR?

@slava77
Copy link
Contributor

slava77 commented Aug 25, 2015

this was already auto-merged from 75X #10777 with 9d4aa93
it's available in 76X starting from CMSSW_7_6_X_2015-08-20-2300

@dgulhan please check that all changes are in place and close this PR once confirmed.

davidlange6 added a commit that referenced this pull request Aug 26, 2015
Pixel cluster splitting and jet core iteration for HI Tracking
@davidlange6 davidlange6 merged commit 8ed8251 into cms-sw:CMSSW_7_6_X Aug 26, 2015
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