Navigation Menu

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

include pilot blade stuff to pixel rawdigi, 2nd attempt #5961

Merged
merged 2 commits into from Oct 27, 2014

Conversation

dkotlins
Copy link
Contributor

Include pilot-blade in raw2digi.
Add the new data format for phase1 pixels.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @dkotlins for CMSSW_7_3_X.

include pilot blade stuff to pixel rawdigi, 2nd attempt

It involves the following packages:

EventFilter/SiPixelRawToDigi

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @VinInn 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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@@ -97,6 +98,11 @@ class PixelDataFormatter {
int hasDetDigis;
ErrorChecker errorcheck;

// For the 32bit data format (moved from ccnamespace)
int ADC_shift, PXID_shift, DCOL_shift, ROC_shift, LINK_shift;
Copy link
Contributor

Choose a reason for hiding this comment

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

CMS style is to have all class members to start from a lower case;
upper case is reserved for type names

Copy link
Contributor

Choose a reason for hiding this comment

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

ALL Tracker code in EventFilter use this type of notation. It is historical. Make no sense to change it 20 year after.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's OK for constants (and they were for 20 years and we are changing them)
They are not constants anymore here.
I would not insist though.

@slava77
Copy link
Contributor

slava77 commented Oct 25, 2014

Since this appears to be just a temporary solution, Danek, please update the description of the pull request to clarify this.
Thanks.

@slava77
Copy link
Contributor

slava77 commented Oct 25, 2014

Based on our reco tests this PR is fine.
I will wait for just the easy fix: removal of the private afs/ from the test files

@slava77
Copy link
Contributor

slava77 commented Oct 27, 2014

+1

for #5961 c39b12e
tests were based on #5961 1ed3c7d
done in CMSSW_7_3_X_2014-10-25-0200 (test are sign444)

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes or unless it breaks tests.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

cmsbuild added a commit that referenced this pull request Oct 27, 2014
include pilot blade stuff to pixel rawdigi, 2nd attempt
@cmsbuild cmsbuild merged commit f184df9 into cms-sw:CMSSW_7_3_X Oct 27, 2014
@cmsbuild
Copy link
Contributor

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