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

Fix rotation for RPC chambers in ring 4 #4752

Merged
merged 1 commit into from Oct 25, 2014

Conversation

ianna
Copy link
Contributor

@ianna ianna commented Jul 23, 2014

  • Match rotation to existing chambers

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ianna for CMSSW_7_1_X.

Fix rotation for RPC chambers in ring 4

It involves the following packages:

Geometry/MuonCommonData

@civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @nclopezo, @Degano, @ktf can you please review it and eventually sign? Thanks.
@ghellwig 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.

@civanch
Copy link
Contributor

civanch commented Jul 23, 2014

@ianna, should the fix go also into 7_2_X?

@ianna
Copy link
Contributor Author

ianna commented Jul 23, 2014

@civanch - yes, if the tests run by Piet are ok - to be confirmed today/tomorrow. GTs would need to be updated as well.

@cmsbuild
Copy link
Contributor

@ktf
Copy link
Contributor

ktf commented Jul 23, 2014

@ianna can you please sign off as well?

@davidlange6
Copy link
Contributor

Not yet please

On Jul 23, 2014, at 6:21 PM, "Giulio Eulisse" <notifications@github.commailto:notifications@github.com> wrote:

@iannahttps://github.com/ianna can you please sign off as well?


Reply to this email directly or view it on GitHubhttps://github.com//pull/4752#issuecomment-49897725.

@ianna
Copy link
Contributor Author

ianna commented Jul 23, 2014

@ktf - I'm waiting for Piet to test this and confirm that it fixes the problem.

@cmsbuild
Copy link
Contributor

@ianna
Copy link
Contributor Author

ianna commented Jul 25, 2014

@pietverwilligen - ping!

@pietverwilligen
Copy link
Contributor

I know, trying since the beginning to see what is wrong. From the tester
everything seems fine, when I reconstruct data i do not see a difference.
greets
Piet

On Fri, Jul 25, 2014 at 1:39 PM, ianna notifications@github.com wrote:

@pietverwilligen https://github.com/pietverwilligen - ping!


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

Piet Verwilligen

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

@pietverwilligen
Copy link
Contributor

Hi Yana

There is something I do not understand. When checking the geometry in
Geometry/RPCGeometry/test things seem to be fine. The position of the first
strip in phi has changed and the numbering goes in the opposite way. For
this I loaded in the configfile of the tester:

process.load('Configuration.Geometry.GeometryExtended2015_cff')

And i did a manual check that it is reading the right rpcf.xml file and
indeed is is reading Geometry/MuonCommonData/data/v2/rpcf.xml, exactly the
file we modified. So up to here everything is ok.

If I then do the reconstruction loading also the same config file:

process.load('Configuration.Geometry.GeometryExtended2015_cff')

I get the rechits reconstructed at exactly the same place, so this is not
ok. So I wanted to check manually whether the geometry is fine with
cmsShow. For this I created again cmsSimGeom-14.root by copying from
Fireworks/Geometry/python/dumpSimGeometry_cfg.py (*) and I replaced the
loading of the default geometry (IdealGeometryXML that does not seem to
load rpcf.xml) with again

process.load('Configuration.Geometry.GeometryExtended2015_cff')

If i go navigating the table I see that RE+4 is still RTXU instead of
RTXUR. Can you remind me how you created the SimGeometry? Now that I could
not see it in the geometry i dont know whether I can really trust the
reconstruction I did ...

(*) btw I am working in 711 (data taking MWGR1) but I could not check out
Fireworks/Geometry for this release, so i took it from 710. I was not
expecting that the 711 release does not have Fireworks/Geometry.

greets
Piet

On Fri, Jul 25, 2014 at 2:05 PM, Piet Verwilligen piet.verwilligen@cern.ch
wrote:

I know, trying since the beginning to see what is wrong. From the tester
everything seems fine, when I reconstruct data i do not see a difference.
greets
Piet

On Fri, Jul 25, 2014 at 1:39 PM, ianna notifications@github.com wrote:

@pietverwilligen https://github.com/pietverwilligen - ping!


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

Piet Verwilligen

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

Piet Verwilligen

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

@ianna
Copy link
Contributor Author

ianna commented Jul 25, 2014

@pietverwilligen you need to add transient reco geometry as well, otherwise the one from DB is picked up:

process.load('Configuration.Geometry.GeometryExtended2015_cff')
process.load('Configuration.Geometry.GeometryExtended2015Reco_cff')

@pietverwilligen
Copy link
Contributor

Thanks that worked, now I see indeed RTXUR for RE+4.

On Fri, Jul 25, 2014 at 3:19 PM, ianna notifications@github.com wrote:

@pietverwilligen https://github.com/pietverwilligen you need to add
transient reco geometry as well, otherwise the one from DB is picked up:

process.load('Configuration.Geometry.GeometryExtended2015_cff')
process.load('Configuration.Geometry.GeometryExtended2015Reco_cff')


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

Piet Verwilligen

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

@pietverwilligen
Copy link
Contributor

Hi Yana

I think that the fix in principle is ok. and I suspect that it is actually
not solving our problem because also Raw2Digi needs to pick up this
geometry. And I am wondering how to test that....

Is it possible to propagate this fix into the DB Geometry and make a label
(and a new Global Tag) such that the Raw2Digi picks up the correct
geometry? I will write an email to our Raw2Digi contact and the AlCa
conveners.
greets
Piet

On Fri, Jul 25, 2014 at 3:30 PM, Piet Verwilligen piet.verwilligen@cern.ch
wrote:

Thanks that worked, now I see indeed RTXUR for RE+4.

On Fri, Jul 25, 2014 at 3:19 PM, ianna notifications@github.com wrote:

@pietverwilligen https://github.com/pietverwilligen you need to add
transient reco geometry as well, otherwise the one from DB is picked up:

process.load('Configuration.Geometry.GeometryExtended2015_cff')
process.load('Configuration.Geometry.GeometryExtended2015Reco_cff')


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

Piet Verwilligen

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

Piet Verwilligen

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

@alja
Copy link
Contributor

alja commented Sep 16, 2014

+1
This change is necessary for visualization too.

@alja
Copy link
Contributor

alja commented Sep 16, 2014

@ianna

Is this correction needed also in 72 ?

@ianna
Copy link
Contributor Author

ianna commented Sep 16, 2014

@alja - yes, when this is in 71x

@ianna
Copy link
Contributor Author

ianna commented Sep 16, 2014

@davidlange6 - it looks like this one is forgotten.

@davidlange6
Copy link
Contributor

@ianna - its not forgotten - in 71x - I want to change all of the geometry at the same time - this and the tracker material model

@ianna
Copy link
Contributor Author

ianna commented Sep 17, 2014

@davidlange6 - it is already used in GR_P_V47 for prompt reco. The xml in the release is out of date.

@davidlange6
Copy link
Contributor

Yes, I'm aware. Its the MC that I am worried about.

On Sep 17, 2014, at 10:57 AM, Ianna Osborne notifications@github.com
wrote:

@davidlange6 - it is already used in GR_P_V47 for prompt reco. The xml in the release is out of date.


Reply to this email directly or view it on GitHub.

@ktf
Copy link
Contributor

ktf commented Oct 16, 2014

shall we close this?

@davidlange6
Copy link
Contributor

+1

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

7 participants