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

Save pixel and strip layers in miniAOD #15605

Merged
merged 6 commits into from Aug 29, 2016

Conversation

gpetruc
Copy link
Contributor

@gpetruc gpetruc commented Aug 25, 2016

Save also the strip and pixel layers for packed candidates.
For compactness, instead of saving then also the number of hits we save the number of extra hits (hits - layers).

Disk size cost seems to be ~100 bytes/event, on 8.5k TTbar (PUpmx25ns).

Validation module updated as well, and used to check that there is perfect agreement between the reported number of hits and layers in the strip and pixel detector between the AOD track, the PackedCandidate methods and the PackedCandidate pseudoTrack, on ~1k TTbar events, both for 2016 and 2017 scenarios.

When reading old MiniAODs, the number of layers will be reported as zero, and but the number of hits should still be correct (I didn't put any iorule magic to try to fudge a number of layers).

@arizzi @makortel @ferencek

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @gpetruc (Giovanni Petrucciani) for CMSSW_8_1_X.

It involves the following packages:

DQM/TrackingMonitor
DataFormats/PatCandidates

@cvuosalo, @dmitrijus, @cmsbuild, @montjj, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@hdelanno, @makortel, @fioriNTU, @cbernet, @idebruyn, @threus 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

@arizzi
Copy link
Contributor

arizzi commented Aug 25, 2016

I guess we need an IORULE to correctly fill nHits when reading old files, isn't it?

@gpetruc
Copy link
Contributor Author

gpetruc commented Aug 25, 2016

As written in the PR description, no iorule should be needed to have the
right number of hits when reading the old miniAODs.

On Thu, Aug 25, 2016 at 2:15 PM, arizzi notifications@github.com wrote:

I guess we need an IORULE to correctly fill nHits when reading old files,
isn't it?


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

@arizzi
Copy link
Contributor

arizzi commented Aug 25, 2016

ah right, apologies for reading the code only instead of the description :-)

On Thu, Aug 25, 2016 at 2:24 PM, Giovanni Petrucciani <
notifications@github.com> wrote:

As written in the PR description, no iorule should be needed to have the
right number of hits when reading the old miniAODs.

On Thu, Aug 25, 2016 at 2:15 PM, arizzi notifications@github.com wrote:

I guess we need an IORULE to correctly fill nHits when reading old files,
isn't it?


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


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

@makortel
Copy link
Contributor

Thanks for updating also the validation code.

if (numberOfPixelHits_ > trackPixelHitsMask) numberOfPixelHits_ = trackPixelHitsMask;
int numberOfStripHits_ = tk.hitPattern().numberOfValidHits() - numberOfPixelHits_;
int numberOfStripHits_ = tk.hitPattern().numberOfValidHits() - numberOfPixelHits_ - numberOfPixelLayers_ - numberOfStripLayers_;
Copy link
Contributor

Choose a reason for hiding this comment

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

some comments about the incremental meaning of values may be useful

@slava77
Copy link
Contributor

slava77 commented Aug 25, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 25, 2016

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

@gpetruc
Copy link
Contributor Author

gpetruc commented Aug 25, 2016

ok, I can add some the comments after the tests are done

On Thu, Aug 25, 2016 at 4:28 PM, cmsbuild notifications@github.com wrote:

The tests are being triggered in jenkins.


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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@gpetruc
Copy link
Contributor Author

gpetruc commented Aug 26, 2016

Added comments explaining the logic, as per Slava's comment.
The new commit 45fe985 only adds comments, so you don't need to rerun the tests

@cmsbuild
Copy link
Contributor

Pull request #15605 was updated. @cvuosalo, @dmitrijus, @cmsbuild, @montjj, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Aug 26, 2016

@gpetruc
are you planning a backport to 80X for the rereco?

@slava77
Copy link
Contributor

slava77 commented Aug 26, 2016

@cmsbuild please test
[even though it's cosmetic]

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 26, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #15605 45fe985

Adding pixel and strip layers to packed candidates in Mini-AOD.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-08-26-1100 show no significant differences, as expected. The new DQM histograms do appear in the Jenkins output DQM files. An extended test of workflow 136.731_RunSinglePh2016B with 20 events against baseline CMSSW_8_1_0_pre10 shows no significant differences, and the additions to the packed candidates do show up in the Mini-AOD output:

   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
  12621.6 ->     12753.5        132      1.0   0.09     patPackedCandidates_packedPFCandidates__reRECO.
   1348.5 ->      1398.5         50      3.6   0.03     patPackedCandidates_lostTracks__reRECO.
-------------------------------------------------------------
   145446 ->      145626        180             0.1     ALL BRANCHES

@dmitrijus
Copy link
Contributor

+1

@davidlange6 davidlange6 merged commit 5d19ae7 into cms-sw:CMSSW_8_1_X Aug 29, 2016
@ferencek
Copy link
Contributor

This was already asked but I have not seen a clear reply. @gpetruc, will there be a 80X backport of this PR?

@gpetruc
Copy link
Contributor Author

gpetruc commented Aug 30, 2016

Yep, it's #15641

On Tue, Aug 30, 2016 at 11:42 AM, Dinko Ferencek notifications@github.com
wrote:

This was already asked but I have not seen a clear reply. @gpetruc
https://github.com/gpetruc, will there be a 80X backport of this PR?


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

@ferencek
Copy link
Contributor

Perfect, thanks. I was not getting GitHub emails about the backport PR
so wasn't sure if it existed but now I also see that the backport PR
references this PR so I would have answered my question if I looked more
carefully.

On 08/30/2016 12:49 PM, Giovanni Petrucciani wrote:

Yep, it's #15641

On Tue, Aug 30, 2016 at 11:42 AM, Dinko Ferencek notifications@github.com
wrote:

This was already asked but I have not seen a clear reply. @gpetruc
https://github.com/gpetruc, will there be a 80X backport of this PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15605 (comment),
or mute
the thread

https://github.com/notifications/unsubscribe-auth/AEbbR8PNvr-WbfthRRKn4tnovEdMu3RUks5qk_sbgaJpZM4Js_Ku
.


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

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

9 participants