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

Customization for merging General and Pixel Track Collections in PbPb Reconstruction #14819

Merged
merged 5 commits into from Jun 28, 2016

Conversation

abaty
Copy link
Contributor

@abaty abaty commented Jun 8, 2016

This PR adds a customization that merges tracks from the HIGeneralTracks and HIConformalPixelTracks collections into a single merged collection, which will be needed in a future reReco. This customization drastically improves the tracking efficiency and fake rate at pt's less than 2 GeV, and will significantly enhance physics capabilities in HI events at low pt.

The customization can be checked running a config generated with the command below over RAW data from the 2015 PbPb run. Note that with this customization the previously used HIGeneralTracks collection is dropped and replaced with a MergedCollection in the final event content.

cmsDriver.py step2 --process RECO --repacked --conditions 75X_dataRun2_PromptHI_v3 --scenario HeavyIons -s RAW2DIGI,L1Reco,RECO --datatier AOD --customise Configuration/DataProcessing/RecoTLR.customiseRun2CommonHI,RecoHI/Configuration/RecoMergedTrackCollection.customiseAddMergedTrackCollection --data --eventcontent AOD --no_exec

More information regarding the changes in performance can be found in the presentation shown in the HI PAG meeting here:
https://indico.cern.ch/event/507212/contributions/2189474/attachments/1284769/1910223/Pixel_Status_HIN_WEST.pdf

@CesarBernardes

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2016

A new Pull Request was created by @abaty for CMSSW_7_5_X.

It involves the following packages:

RecoHI/Configuration

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

@slava77
Copy link
Contributor

slava77 commented Jun 8, 2016

@cmsbuild please test

@abaty please clarify on plans when this is needed (75X are built on demand now, the last one was more than 2 months ago).

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2016

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

@BetterWang
Copy link
Contributor

Dear @slava77 , we would like to have a release to re-reco last year's PbPb data including pixel tracks.
This can extend our low pT reach and reduce fake rate significantly.

@slava77
Copy link
Contributor

slava77 commented Jun 8, 2016

On 6/8/16 7:51 AM, Quan Wang wrote:

Dear @slava77 https://github.com/slava77 , we would like to have a
release to re-reco last year's PbPb data including pixel tracks.
This can extend our low pT reach and reduce fake rate significantly.

when?

  • within a week
  • some time before pPb data taking
  • by the end of the year

Do you expect any more changes needed for this release?
There is time in RECO meeting next week for HI topics.
Please plan to cover the 75X needs there.


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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2016

@mandrenguyen
Copy link
Contributor

@abaty Is there any reason not to add this to the frontier release (81X)? Or is this the first and last time we will ever run pixel tracking? What is the expected impact on downstream products such as muons and particle flow?

@abaty
Copy link
Contributor Author

abaty commented Jun 13, 2016

@mandrenguyen @CesarBernardes

In the future I believe we might promote the merged track collection the default track reconstruction for HI, but this needs more discussion in a wider audience I think. We should prepare a PR for the new release in any case.

The current customization in this PR implementation should not affect anything downstream, as the building of the HIGeneralTracks collection has not changed at all. The downstream products will still use this collection as input, and then the collection will be dropped at the end of the processing, in favor of keeping the merged collection. In the future if it is decided we want to move towards using the merged collection for everything, we will need to make sure all the downstream stuff picks up the new collection.

@mandrenguyen
Copy link
Contributor

@abaty Making this the default behavior is a separate issue from having this in 81X. I'm somewhat wary of dropping hiGeneralTracks, since other collections will contain references to this collection. Given the very limited scope of this reprocessing (two primary datasets, I've been told), is it really worth dropping the standard track collection, which is being used by many downstream modules? Perhaps @BetterWang can comment for the flow group.

@abaty
Copy link
Contributor Author

abaty commented Jun 13, 2016

@mandrenguyen

If you are strongly against dropping this collection for now then I think we can keep it; we were trying to limit the amount of disk space used. As you said since this is a limited reprocessing then the extra space hopefully won't be a huge issue for now.

@BetterWang
Copy link
Contributor

Hi @abaty @mandrenguyen @CesarBernardes ,
The first round of reprocessing will be one HIMB PD and one HIFlowCorr PD. After that, we plan to reprocess all MB PD. The disk usage is quite heavy though. I will ask other PinG leaders if they need to reprocess other PDs to do further studies. I would like to keep hiGeneralTracks, but it really depends on the disk space at VU.

@mandrenguyen
Copy link
Contributor

mandrenguyen commented Jun 13, 2016

@BetterWang That's not what was proposed to me by the HI conveners. I would suggest a clear plan that includes an estimate of the resources required be presented at the PAG meeting. I find it hard to believe that the other sub-group leaders will/have signed off on a reprocessing of the entire dataset without storing the standard track collection, which will almost certainly crash our standard ntuples (hiForest).

@cmsbuild
Copy link
Contributor

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

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 17, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

Testing in progress...

@cvuosalo
Copy link
Contributor

@abaty: It appears the instructions for running the test config are wrong. The customise argument is given as:

--customise Configuration/DataProcessing/RecoTLR.customiseRun2CommonHI,RecoHI/Configuration/RecoMergedTrackCollection.customiseAddMergedTrackCollection

but this causes an error because there is no file called RecoMergedTrackCollection. The following customise argument works:

 --customise Configuration/DataProcessing/RecoTLR.customiseRun2CommonHI,RecoHI/Configuration/customise_RecoMergedTrackCollection.customiseAddMergedTrackCollection

Are the instructions incorrect, or does the file in this PR have the wrong name?

@BetterWang
Copy link
Contributor

@cvuosalo The documentation was out dated.

@cvuosalo
Copy link
Contributor

+1

For #14819 4b0b8d9

Adding customization for merging General and Pixel Track Collections in PbPb reco. There should be no change in standard workflows since this PR adds a new config that is not used by default.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_7_5_X_2016-06-12-0000 show no significant differences, as expected, though there are a few spurious differences in the validation plots. The following test exercised the new config with 10 events against baseline CMSSW_7_5_8_patch4:

cmsDriver.py step2 --process RECO --repacked --conditions 75X_dataRun2_PromptHI_v3 --scenario HeavyIons -s RAW2DIGI,L1Reco,RECO --datatier AOD --customise Configuration/DataProcessing/RecoTLR.customiseRun2CommonHI,RecoHI/Configuration/customise_RecoMergedTrackCollection.customiseAddMergedTrackCollection --data --eventcontent AOD --customise Validation/Performance/TimeMemoryInfo.py --filein /store/hidata/HIRun2015/HIMinimumBias2/RAW/v1/000/262/818/00000/006CDFEB-2598-E511-9D75-02163E014781.root --fileout file:step2.root  -n 10

The collections added by this PR increase Reco event content by about 7%:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
      0.0 ->       343.8        344     NEWO   0.03     floatedmValueMap_hiGeneralAndPixelTracks_MVAVals_RECO.
      0.0 ->     69678.1      69678     NEWO   6.93     recoTracks_hiGeneralAndPixelTracks__RECO.
      0.0 ->       344.2        344     NEWO   0.03     floatedmValueMap_hiPixelOnlyStepSelector_MVAVals_RECO.
      0.0 ->       339.4        339     NEWO   0.03     floatedmValueMap_hiHighPtStepSelector_MVAVals_RECO.
-------------------------------------------------------------
  1005691 ->     1076412      70721             7.0     ALL BRANCHES

CPU time increases by about 11%.

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
       added      +0.40%         0.00 ms/ev ->        74.28 ms/ev hiGeneralAndPixelTracks
       added      +8.73%         0.00 ms/ev ->      1612.74 ms/ev hiConformalPixelTracks
       added      +0.02%         0.00 ms/ev ->         3.27 ms/ev hiPixelOnlyStepSelector
       added      +0.02%         0.00 ms/ev ->         2.91 ms/ev hiHighPtStepSelector
  ---------- ------------     --------                  ----       ------------
Job total:  18.4635 s/ev ==> 20.4358 s/ev

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_7_6_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 7500dd9 into cms-sw:CMSSW_7_5_X Jun 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

8 participants