Separate 2015 Geometry including the 4 installed RE1/1 chambers #8855

pietverwilligen · 2015-04-23T15:38:06Z

No description provided.

PRToFixMess_RPC4RE11Geometry_PR

…cmssw into RPC4RE11_75X_PR

cmsbuild · 2015-04-23T15:41:24Z

A new Pull Request was created by @pietverwilligen (Piet Verwilligen) for CMSSW_7_5_X.

Separate 2015 Geometry including the 4 installed RE1/1 chambers

It involves the following packages:

Configuration/Geometry
DataFormats/MuonDetId
Fireworks/Geometry
Geometry/CMSCommonData
Geometry/MuonCommonData
Geometry/MuonSimData
Geometry/RPCGeometry
Geometry/RPCGeometryBuilder

@civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @alja, @nclopezo, @ktf can you please review it and eventually sign? Thanks.
@ghellwig, @battibass, @abbiendi, @jhgoh, @Martin-Grunewald, @alja, @trocino 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.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

pietverwilligen · 2015-04-23T15:41:45Z

This PR contains an separate geometry (variant on 2015 geometry) where the 4 RE1/1 chambers installed during LS1 are included. One modification to the RPCDetId had to be done to avoid a rotation by 10 degrees. I checked the list of DetId before and after the change in DataFormats/MuonDetId/src/RPCDetId.cc and the list are (apart from the 4 new chambers) exactly the same.

ianna · 2015-04-24T15:47:58Z

DataFormats/MuonDetId/src/RPCDetId.cc

+    if ( !(ring == 1 && station > 1 && region==1)) {     
+      sector_id+=1;
+      if (sector_id==37)sector_id=1;
+    }


@pietverwilligen - you are changing DetId numbering. Wouldn't it invalidate previously taken RPC data and simulation production?

Hi Yana

I printed a list of DetIds before and after changing ... The DetIds and the
associated RPC Chamber names are exactly the same.
I have run (an only 10 events) tests and the sim+digi+rechit+dqm chain
gives the same results.

As far as I understand this piece of code it introduces an arbitrarily
rotation in
RE+1/(1,2,3), RE-1/(2,3), RE+/-(2,3,4)/2,3
it seems that for rings filled with 36 chambers there is no change, while
there is a change if you only put 4 RE+1/1 chambers.

I can actually come up with a solution in which I modify that piece of code
to take out only the RE+1/1 chambers affected by this code. Although that
code of code is rather untransparent to me I ll be able to leave the code
for the existing chambers (RE+/-(1-4)/(2,3) untouched.
greets
Piet

(*) that piece of code act differently on RE+(2,3,4)/1 and RE-(2,3,4)/1,
makes me a bit suspicious... I cannot think of any asymmetry between
positive and negative endcap that would only affect RE+/-(2,3,4)/1 and no
other chambers... but it can exist.

On Fri, Apr 24, 2015 at 5:48 PM, Ianna Osborne notifications@github.com
wrote:

In DataFormats/MuonDetId/src/RPCDetId.cc
#8855 (comment):

if (region!=0) {

if ( !(ring == 1 && station > 1 && region==1)) {

sector_id+=1;

if (sector_id==37)sector_id=1;

}

if ( !(ring == 1 && station > 1 && region==1)) {

sector_id+=1;

if (sector_id==37)sector_id=1;

}

@pietverwilligen https://github.com/pietverwilligen - you are changing
DetId numbering. Wouldn't it invalidate previously taken RPC data and
simulation production?

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/8855/files#r29060113.

Piet Verwilligen

INFN -- Sezione di Bari
Via E. Orabona 4
I-70125 Bari, Italy
Phone: +39 345 74 70 642

pietverwilligen · 2015-04-24T16:20:02Z

I have updated the PR. Yana, can you see whether you like this?
greets
PIet

cmsbuild · 2015-04-24T16:21:24Z

Pull request #8855 was updated. @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @alja, @nclopezo, @ktf can you please check and sign again.

ianna · 2015-04-25T07:23:58Z

+1

pietverwilligen · 2015-04-27T09:47:23Z

Hi All

Can someone who is more expert than I am have a look, or tell me more
precisely where I should look at? Or tell me whether this is something to
be worried about or not?

I checked the workflow 140.53 and I saw that only DQM had failing
comparisons (39/92):
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_5_X_2015-04-24-2300+8855/4390/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/

Then also looking at the DQM root files:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_5_X_2015-04-24-2300+8855/4390/files/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/
There I checked the RPC histograms for the reference and for the new run
and I saw they were equal ...

Thanks a lot
greets
Piet

On Sat, Apr 25, 2015 at 11:34 AM, cmsbuild notifications@github.com wrote:

Comparison is ready

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8855/3428/summary.html

The workflows 140.53 have different files in step1_dasquery.log than the
ones found in the baseline. You may want to check and retrigger the tests
if necessary. You can check it in the "files" directory in the results of
the comparisons

—
Reply to this email directly or view it on GitHub
#8855 (comment).

Piet Verwilligen

INFN -- Sezione di Bari
Via E. Orabona 4
I-70125 Bari, Italy
Phone: +39 345 74 70 642

slava77 · 2015-04-27T10:58:49Z

Hi Piet,

It could be "The workflows 140.53 have different files in
step1_dasquery.log than ... "
is a false-positive for the purpose of the comparisons:
if the first file happened to be the same,
then the job runs on the same events for the baseline and the tested code.

    --slava

On 4/27/15 4:47 AM, Piet Verwilligen wrote:

Hi All

Can someone who is more expert than I am have a look, or tell me more
precisely where I should look at? Or tell me whether this is something to
be worried about or not?

I checked the workflow 140.53 and I saw that only DQM had failing
comparisons (39/92):
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_5_X_2015-04-24-2300+8855/4390/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/

Then also looking at the DQM root files:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_5_X_2015-04-24-2300+8855/4390/files/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/
There I checked the RPC histograms for the reference and for the new run
and I saw they were equal ...

Thanks a lot
greets
Piet

On Sat, Apr 25, 2015 at 11:34 AM, cmsbuild notifications@github.com wrote:

Comparison is ready

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8855/3428/summary.html

The workflows 140.53 have different files in step1_dasquery.log than the
ones found in the baseline. You may want to check and retrigger the tests
if necessary. You can check it in the "files" directory in the results of
the comparisons

—
Reply to this email directly or view it on GitHub
#8855 (comment).

Piet Verwilligen

INFN -- Sezione di Bari
Via E. Orabona 4
I-70125 Bari, Italy
Phone: +39 345 74 70 642

—
Reply to this email directly or view it on GitHub
#8855 (comment).

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

civanch · 2015-04-27T14:38:22Z

@pietverwilligen , can you, please, double check that RPCDetId.cc is working as desired? My concern to the lines 179-198: in the block of lines 179-187 the computation of sector_id is performed for (region!=0), however, in the line 182 the same check (region!=0) is applied; after that in lines 185-195 sector_id is changed for (region==-1). Are these lines consistent?

pietverwilligen · 2015-04-27T15:46:48Z

Hi Vladimir

My main concern was also those lines. The only thing I did here was to
take out RE+1/1 which are not included in any geometry. It is indeed messy,
I inherited it in this way let' s say. Now this PR is not a clean up of the
situation (although that should be done at some point). I checked
explicitly and more than once that RPCDetId.cc is working as desired.

Now your point about line 182 is correct, I should not have required again
the statement "region!=0", just "if(!(ring == 1 && station == 1)" is
enough. Do you want me to take it out?
greets
Piet

On Mon, Apr 27, 2015 at 4:38 PM, Vladimir Ivantchenko <
notifications@github.com> wrote:

@pietverwilligen https://github.com/pietverwilligen , can you, please,
double check that RPCDetId.cc is working as desired? My concern to the
lines 179-198: in the block of lines 179-187 the computation of sector_id
is performed for (region!=0), however, in the line 182 the same check
(region!=0) is applied; after that in lines 185-195 sector_id is changed
for (region==-1). Are these lines consistent?

—
Reply to this email directly or view it on GitHub
#8855 (comment).

Piet Verwilligen

INFN -- Sezione di Bari
Via E. Orabona 4
I-70125 Bari, Italy
Phone: +39 345 74 70 642

civanch · 2015-04-27T16:38:23Z

Hi Piet,
I more worry about the next block were sector_id is redefined for region==-1. If this is OK from your test I can sign this PR, if there is a possibility to get wrong id then better to clean line 182 as well.

pietverwilligen · 2015-04-27T17:37:07Z

Hi Vladimir

The next block where you are looking at is not touched w.r.t. the original
code. To me it also looks suspicious, but this is how we have run the past
3 years during Run-I.
My tests said that all is ok. I compared the list of detids before and
after PR and they were identical.
greets
Piet

On 27 Apr 2015 18:38, "Vladimir Ivantchenko" notifications@github.com
wrote:

Hi Piet,
I more worry about the next block were sector_id is redefined for
region==-1. If this is OK from your test I can sign this PR, if there is a
possibility to get wrong id then better to clean line 182 as well.

—
Reply to this email directly or view it on GitHub
#8855 (comment).

civanch · 2015-04-27T17:44:01Z

+1

civanch · 2015-04-28T15:38:44Z

+1

Separate 2015 Geometry including the 4 installed RE1/1 chambers

pietverwilligen added 15 commits April 23, 2015 13:27

2015 Geometry with 4 RE1/1 chambers

1892cf5

2015 Geometry with 4 RE1/1 chambers

5f1351d

2015 Geometry with 4 RE1/1 chambers

fb94f80

2015 Geometry with 4 RE1/1 chambers

42b3089

2015 Geometry with 4 RE1/1 chambers

882a7d7

2015 Geometry with 4 RE1/1 chambers

254a9fe

2015 Geometry with 4 RE1/1 chambers

3e78bf6

2015 Geometry with 4 RE1/1 chambers

3e66250

2015 Geometry with 4 RE1/1 chambers

3f98831

2015 Geometry with 4 RE1/1 chambers

4708c64

2015 Geometry with 4 RE1/1 chambers

b878cad

2015 Geometry with 4 RE1/1 chambers

243f7f5

Merge pull request #5 from cms-sw/CMSSW_7_5_X

226d803

PRToFixMess_RPC4RE11Geometry_PR

create separate detid list

d0600cb

Merge branch 'RPC4RE11_75X_PR' of https://github.com/pietverwilligen/…

98c494e

…cmssw into RPC4RE11_75X_PR

cmsbuild added this to the Next CMSSW_7_5_X milestone Apr 23, 2015

cmsbuild added comparison-pending geometry-pending orp-pending pending-signatures simulation-pending tests-pending visualization-pending labels Apr 23, 2015

ianna reviewed Apr 24, 2015
View reviewed changes

reintroduce rotation except for RE+1/1

0bd3687

cmsbuild added geometry-approved orp-pending pending-signatures simulation-pending tests-approved visualization-pending labels Apr 25, 2015

cmsbuild added comparison-available geometry-approved orp-pending pending-signatures simulation-approved tests-approved visualization-pending and removed comparison-available geometry-approved orp-pending pending-signatures simulation-pending tests-approved visualization-pending labels Apr 28, 2015

davidlange6 added a commit that referenced this pull request May 3, 2015

Merge pull request #8855 from pietverwilligen/RPC4RE11_75X_PR

b1daec6

Separate 2015 Geometry including the 4 installed RE1/1 chambers

davidlange6 merged commit b1daec6 into cms-sw:CMSSW_7_5_X May 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate 2015 Geometry including the 4 installed RE1/1 chambers #8855

Separate 2015 Geometry including the 4 installed RE1/1 chambers #8855

pietverwilligen commented Apr 23, 2015

cmsbuild commented Apr 23, 2015

pietverwilligen commented Apr 23, 2015

ianna Apr 24, 2015

pietverwilligen Apr 24, 2015

pietverwilligen commented Apr 24, 2015

cmsbuild commented Apr 24, 2015

ianna commented Apr 25, 2015

pietverwilligen commented Apr 27, 2015

slava77 commented Apr 27, 2015

civanch commented Apr 27, 2015

pietverwilligen commented Apr 27, 2015

civanch commented Apr 27, 2015

pietverwilligen commented Apr 27, 2015

civanch commented Apr 27, 2015

civanch commented Apr 28, 2015

Separate 2015 Geometry including the 4 installed RE1/1 chambers #8855

Separate 2015 Geometry including the 4 installed RE1/1 chambers #8855

Conversation

pietverwilligen commented Apr 23, 2015

cmsbuild commented Apr 23, 2015

pietverwilligen commented Apr 23, 2015

ianna Apr 24, 2015

Choose a reason for hiding this comment

pietverwilligen Apr 24, 2015

Choose a reason for hiding this comment

pietverwilligen commented Apr 24, 2015

cmsbuild commented Apr 24, 2015

ianna commented Apr 25, 2015

pietverwilligen commented Apr 27, 2015

slava77 commented Apr 27, 2015

civanch commented Apr 27, 2015

pietverwilligen commented Apr 27, 2015

civanch commented Apr 27, 2015

pietverwilligen commented Apr 27, 2015

civanch commented Apr 27, 2015

civanch commented Apr 28, 2015