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

Addition of pixel Tracking in HI pp_on_AA_2018 era #24257

Merged
merged 4 commits into from Aug 22, 2018

Conversation

abaty
Copy link
Contributor

@abaty abaty commented Aug 9, 2018

This PR creates a new era which is a copy of Run2_2018_pp_on_AA, and adds a pixel tracking sequence to the reconstruction. This pixel tracking sequence has been requested to meet physics goals requiring low-pt tracks with a higher purity than what is achievable with full tracks. The additional pixel tracking modules have been found to have a negligible impact on the total timing. Storing the extra track collection has been estimated to increase the event content size of AOD by 11%, which is a number the HI PAG is willing to accept. Details of the pixel tracking performance in MB HI events can be found at [1].

The PR can be tested with the normal pp_on_AA_2018 cmsDriver command with the era changed to the 'Run2_2018_pp_on_AA_wPixelTrk'.

[1] https://indico.cern.ch/event/749011/contributions/3098671/attachments/1698408/2736177/pixelUpdate_20180727.pdf

@mandrenguyen

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2018

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

It involves the following packages:

Configuration/Eras
Configuration/StandardSequences
RecoHI/HiTracking
RecoTracker/Configuration

@perrotta, @cmsbuild, @franzoni, @slava77, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@echapon, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @mschrode, @yenjie, @jazzitup, @kurtejung, @gpetruc, @ebrondol, @mandrenguyen, @dgulhan, @yetkinyilmaz this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Aug 9, 2018

The additional pixel tracking modules have been found to have a negligible impact on the total timing.

Why do we then need a new era?
can this new collection be added to the already existing pp_on_AA_2018 ?

@abaty
Copy link
Contributor Author

abaty commented Aug 9, 2018

My understanding is that depending on the final data-taking strategy, there could be a request to only run the pixel tracking on a subset of all the PDs gathered for the HI run in 2018. In order to facilitate running two different sequences on specific PDs, I was told it would be beneficial to keep the pixel tracking as a separate era. Let me know if this is not the case. @slava77

@slava77
Copy link
Contributor

slava77 commented Aug 9, 2018 via email

@mandrenguyen
Copy link
Contributor

@slava77 Based on the decision at the last HIN PAG meeting, we are indeed willing to add 11% to AOD. Regarding the two eras, this was requested for the minbias datasets, so I suggested to make a separate era. This being said, I don't think it was yet excluded that we just store pixel tracks for all PDs. Let us get clarification at tomorrow's PAG meeting and we can update this PR if necessary.

@slava77
Copy link
Contributor

slava77 commented Aug 9, 2018

if we really need this additional scenario for the production/prompt later this year, please also make a relval matrix workflow so that the code can be regularly tested (including this PR).

@abaty
Copy link
Contributor Author

abaty commented Aug 9, 2018

We recently checked the size increase in b=0 events, and it looks like the number is still 11%.

extraHitRPhitolerance = cms.double(0.032),
maxChi2 = cms.PSet(
enabled = cms.bool(True),
pt1 = cms.double(0.7),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please drop type specifications for all parameters which already exist.
This is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.

)

#Pixel tracks
hiConformalPixelTracksPhase1 = cms.EDProducer("PixelTrackProducer",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that "Phase1" can be dropped from the producer name.

@slava77
Copy link
Contributor

slava77 commented Aug 13, 2018

@mandrenguyen
please clarify when you expect to have a decision on this.
Thank you.

@abaty
Copy link
Contributor Author

abaty commented Aug 13, 2018

Based on the HI PAG meeting on Friday, the decision was made to move the pixel tracking into the main pp_on_AA_2018 era. To try to mitigate some of the event size increase, we will also slightly bump up the low-pt threshholds of the generalTracks (as these tracks are mostly fake anyways).

I'm finishing up some tests on the new setup and should push is soon.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@abaty
Copy link
Contributor Author

abaty commented Aug 13, 2018

I've moved the changes in this PR back into the main pp_on_AA_2018 era, as discussed in my previous comment. The pt threshholds for the low pt steps have been raised from 0.3 to 0.49 GeV as well. This reduces the number of tracks under 0.5 GeV stored, reducing the extra event content added to AOD in this PR from 11% down to around 7%. RECO now also runs slightly faster because of the timing saved in the low-pt step.

I've also made changes addressing comments from Slava's code review.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #24257 was updated. @perrotta, @cmsbuild, @franzoni, @slava77, @fabiocos, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Aug 15, 2018

@cmsbuild please test workflow 158.0

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 15, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/29838/console Started: 2018/08/15 16:29

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

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

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-24257/158.0_HydjetQ_B12_5020GeV_2018+HydjetQ_B12_5020GeV_2018+DIGIHI2018PPRECO+RECOHI2018PPRECO+HARVESTHI2018PPRECO

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2891469
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2891278
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@slava77
Copy link
Contributor

slava77 commented Aug 20, 2018

Here are some observations to go with the PR

Using 230 minbias events after the filter "3000":
there is about 10% drop in the number of generalTracks
all_sign1034vsorig_hinpbpbmbfilter3000c_log10recotracks_generaltracks__reco_obj_pt
mostly from low pt.

all_sign1034vsorig_hinpbpbmbfilter3000c_recotracks_generaltracks__reco_obj_algo
low-pt triplet and quad iterations are down, in line with the increased thresholds, but the later iterations are now picking up more tracks (including pixel pair).

particle flow follows with a drop at very low pt and some increase up to 1.5 GeV, from the later iterations; overall 12% fewer charged candidates
all_sign1034vsorig_hinpbpbmbfilter3000c_log10recopfcandidates_particleflow__reco_obj_pt46

the pixel tracks show up as expected. There are about 5K tracks per event (only about 20% less than in the generalTracks collection)
pixeltrack_origvssign1034_hinpbpbmbfilter3000c_log10recotracks_hiconformalpixeltracks__reco_obj_pt

CPU use decreased by about 2%:

  • the tracking time total is roughly unchanged : a reduction in the lowPt iterations is balanced out by the added pixel tracks and also increase in time spent on later iterations.
  • overall decrease in the number of tracks and PF charged candidates reduces time in various high level reco, which adds up to about 2% and explains a total CPU reduction

The event size is up by about 7.5% in AODSIM output

  • hiConformalPixelTracks contribute an increase of about 11%, as expected from the PR description
  • the reduction in generalTracks and pf candidates dominates the overall reduction and adds up to 2.3%

Memory peak in a production-like job with 8 threads increased by 1 GB (from 15 GB to 16 GB). This is apparently the cost of adding the pixel tracks, but is perhaps also partly coming from the increase in iterative tracking seeds and candidates for higher iterations.

Overall, the results appear to be somewhat as expected.

@slava77
Copy link
Contributor

slava77 commented Aug 20, 2018

similar plots for wf 158.0 (200 events):

all_sign1034vsorig_hydjetqb12pprecoin2018wf158p0c_log10recotracks_generaltracks__reco_obj_pt

all_sign1034vsorig_hydjetqb12pprecoin2018wf158p0c_recotracks_generaltracks__reco_obj_algo

all_sign1034vsorig_hydjetqb12pprecoin2018wf158p0c_log10recopfcandidates_particleflow__reco_obj_pt46

there is essentially no visible increase in the number of tracks with pt around 1 GeV. Here the fraction of fakes is apparently much smaller.

The change in the fake rate can be seen from the MTV plots:

  • for the wf 158.0 it did not increase much and remains low

wf158 0_general09_fr

- for the "filter 3000" events it has gone up significantly

wffilter3000_general09_fr

@slava77
Copy link
Contributor

slava77 commented Aug 20, 2018

+1

for #24257 68367e8

@kpedro88
Copy link
Contributor

+1

@kpedro88
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 707c423 into cms-sw:master Aug 22, 2018
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