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 full alignment and APE for Pixel Phase-I detector #14545

Merged
merged 2 commits into from May 19, 2016

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented May 18, 2016

Summary of changes in Global Tags

Upgrade

This should allow to switch on the applyAlignment parameter of trackerGeometry

@mmusich
Copy link
Contributor Author

mmusich commented May 18, 2016

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 18, 2016

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

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmusich (Marco Musich) for CMSSW_8_1_X.

It involves the following packages:

Configuration/AlCa
Configuration/Geometry

@ghellwig, @civanch, @Dr15Jones, @ianna, @cerminar, @cmsbuild, @franzoni, @mdhildreth, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @Martin-Grunewald, @tocheng this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@cmsbuild
Copy link
Contributor

@mmusich
Copy link
Contributor Author

mmusich commented May 18, 2016

+1

@cmsbuild
Copy link
Contributor

@mmusich
Copy link
Contributor Author

mmusich commented May 18, 2016

attn: @makortel

@@ -21,7 +21,7 @@
from Geometry.TrackerGeometryBuilder.idealForDigiTrackerGeometry_cff import *
from Geometry.CSCGeometryBuilder.idealForDigiCscGeometry_cff import *
from Geometry.DTGeometryBuilder.idealForDigiDtGeometry_cff import *
trackerGeometry.applyAlignment = cms.bool(False)
trackerGeometry.applyAlignment = cms.bool(True)
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest to just remove this override instead (I think it would be more clear)? The default is True https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/Geometry/TrackerGeometryBuilder/plugins/TrackerDigiGeometryESModule.cc#L64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@makortel done in the latest commit

@makortel
Copy link
Contributor

Thanks @mmusich!

Sorry to hijack the PR a bit, but on a related note could somebody clarify if the 2017 RECO workflow currently reads the geometry from XML or from GT (RECO step loads "Configuration.Geometry.GeometryExtended2017Reco_cff"; pardon my ignorance on the details of geometry technicalities)? The geometry is in the GTs anyway, right?

@boudoul @ianna

@mmusich
Copy link
Contributor Author

mmusich commented May 18, 2016

@makortel that's my understanding (i.e. geometry taken from GT anyway). Please @ianna confirm

@mmusich
Copy link
Contributor Author

mmusich commented May 18, 2016

$ conddb list 80X_upgrade2017_design_Queue | grep GeometryFileRcd
GeometryFileRcd                                         Extended                                         XMLFILE_Geometry_2017_80YV2_Extended2017_mc                                     
GeometryFileRcd                                         Ideal                                            XMLFILE_Geometry_Run2_76YV2_Ideal2015_mc        

$ conddb list 80X_upgrade2017_realistic_Queue | grep GeometryFileRcd
GeometryFileRcd                                         Extended                                         XMLFILE_Geometry_2017_80YV2_Extended2017_mc                                          
GeometryFileRcd                                         Extended2015CastorMeasured                       XMLFILE_Geometry_80YV1_Extended2015CastorMeasured_mc                                 
GeometryFileRcd                                         Extended2015CastorSystMinus                      XMLFILE_Geometry_80YV1_Extended2015CastorSystMinus_mc                                
GeometryFileRcd                                         Extended2015CastorSystPlus                       XMLFILE_Geometry_80YV1_Extended2015CastorSystPlus_mc                                 
GeometryFileRcd                                         Extended2015ZeroMaterial                         XMLFILE_Geometry_Run2_76YV2_Extended2015ZeroMaterial_mc                              
GeometryFileRcd                                         Extended2015dev                                  XMLFILE_Geometry_Run2_76YV2_Extended2015dev_mc                                       
GeometryFileRcd                                         Ideal                                            XMLFILE_Geometry_Run2_76YV2_Ideal2015_mc                                             
GeometryFileRcd                                         Ideal2015dev                                     XMLFILE_Geometry_Run2_76YV2_Ideal2015dev_mc                                          

also: all the the other geometry labels in GT other than extended are run2 related. I would just remove them in the next iteration. Is it fine with you @ianna?

@mmusich
Copy link
Contributor Author

mmusich commented May 19, 2016

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 19, 2016

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

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

Also, out of general curiosity: is it possible to compare two GTs directly on the new conddb website? This feature was definitely available on the old site...

@mmusich
Copy link
Contributor Author

mmusich commented Jun 1, 2016

@kpedro88 yes it is. Which GT do you want to compare?

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@kpedro88 - yes, you select them, then diff.
I'll check Extended2017 scenario and update it in DB accordingly.

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

Ah, I have to search for the tag to see the diff, I can't just enter the URL directly.

@mmusich
Copy link
Contributor Author

mmusich commented Jun 1, 2016

@ianna @kpedro88 this looks like going back to me. Why is not possible to create new conditions compliant with the latest version of the geometry?

@mmusich
Copy link
Contributor Author

mmusich commented Jun 1, 2016

@kpedro88

https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/GT1/GT2

should fullfill your desires...

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

@mmusich - it is possible to create fake conditions (which may make questionable physical sense), but this is somewhat manpower-intensive at the moment for HCAL. Since a lot of things are still in development, so far we have deemed it easier just to use the hardcode conditions for our work. Pixel development doesn't need to know about the HCAL changes, so better to factorize in my opinion.

Thanks for the diff URL, much easier than using the browser...

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@mmusich - I think, it is actually moving forward :-)

Hcal parameters have been updated recently for 2016, but 2017 scenario was not, though it should have been:

[yana@cmsdev03 src]$ diff Geometry/HcalCommonData/data/Run2/hcalSimNumbering.xml Geometry/HcalCommonData/data/Run2/hcalSimNumbering16.xml
59c59
<      2,  3,  2,  4
---
>      2,  3,  4,  4
68,69c68,71
<   <Vector name="noff" type="numeric" nEntries="18">
<     15, 29,  4, 10,  5,  2,  4, 18, 19, 11, 12, 13, 14,  2,  3,  2,  1,  0
---
>   <Vector name="noff" type="numeric" nEntries="42">
>     15, 29,  4, 10,  5,  2,  4, 18, 19, 11, 12, 13, 14,  2,  3,  2,  1, 12,
>     29, 39, 30, 39, 31, 39, 32, 39, 33, 39, 34, 39, 35, 39, 36, 39,
>     37, 39, 38, 39, 39, 39, 41, 39
87c89
<     2.840, 2.090, 0.000, 0.000
---
>     2.840, 2.090, 2.840, 2.090
[yana@cmsdev03 src]$ diff Geometry/HcalCommonData/data/Run2/hcalRecNumbering.xml Geometry/HcalCommonData/data/Run2/hcalRecNumbering16.xml
40,41c40,41
<     <Parameter name="TopologyMode" value="HcalTopologyMode::LHC" eval="false"/>
<     <Parameter name="TriggerMode"  value="HcalTopologyMode::tm_LHC_RCT" eval="false"/>
---
>     <Parameter name="TopologyMode" value="HcalTopologyMode::SLHC" eval="false"/>
>     <Parameter name="TriggerMode"  value="HcalTopologyMode::tm_LHC_RCT_and_1x1" eval="false"/>

@mmusich
Copy link
Contributor Author

mmusich commented Jun 1, 2016

@kpedro88 , going back and forth with the geometry in GT is manpower-intensive for AlCa, especially considering the current proposal is to step back to: Hcal 2015 + 1x1 HF TPs.
PS: also conddb diff will give you the same output

@mmusich
Copy link
Contributor Author

mmusich commented Jun 1, 2016

@ianna, is the diff you point out supposed to cure the inconsistencies spotted by @makortel in #14545 (comment)?

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

@mmusich - I thought it had been agreed for a long time not to put Extended2017dev in the global tag at all until it was ready. HCAL seems to get yelled at no matter what we do.

@ianna - the changes you point out are for Extended2016dev, with the few new HF QIE10 cells. This was also agreed not to go in the GT until conditions are available. In this case, they hopefully will be soon, but are not yet. @abdoulline may know more about the status for 2016dev.

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@kpedro88 - It's comparing Extended2017 with Extended2016 (no dev)

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

@ianna - ah, this is because of weird historical naming conventions. Extended2016 is actually the dev version, while Extended2016a is the version currently used in 80X, with 2015 HCAL + 1x1 HF TPs. @bsunanda, please confirm.

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@mmusich - yes, this is a fix #14545 (comment)

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@kpedro88 - and Extended2017dev is not dev?

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

@ianna - let me fully clarify the naming situation.

When I'm using the "dev" name, I'm thinking of the Configuration/Geometry configs that actually get loaded by the process. The XML config files are unfortunately not named consistently with the higher-level configs that use them. Neither of the "dev" configs should be in GTs at the moment.

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@kpedro88 - so Extended2017 differs from Extended2016a:

[yana@cmsdev03 src]$ diff Geometry/HcalCommonData/data/Run2/hcalSimNumbering.xml Geometry/HcalCommonData/data/Run2/hcalSimNumbering16a.xml
87c87
<     2.840, 2.090, 0.000, 0.000
---
>     2.840, 2.090, 2.840, 2.090
[yana@cmsdev03 src]$ diff Geometry/HcalCommonData/data/Run2/hcalRecNumbering.xml Geometry/HcalCommonData/data/Run2/hcalRecNumbering16a.xml
41c41
<     <Parameter name="TriggerMode"  value="HcalTopologyMode::tm_LHC_RCT" eval="false"/>
---
>     <Parameter name="TriggerMode"  value="HcalTopologyMode::tm_LHC_RCT_and_1x1" eval="false"/>

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

@bsunanda will have to clarify that. I was not aware of that difference.

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@kpedro88 - yep, we have agreed on naming conventions, but they were not respected for 2016 scenarios:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideDetectorDescription#Adding_New_Geometry_Scenario

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@kpedro88 and @bsunanda - Hcal in Extended2017 scenario is identical to the one in 2015. Plus new phase 1 pixel detector is added. BHM, BCM and PLT are removed. Ideally, current Hcal conditions should work.

@bsunanda
Copy link
Contributor

bsunanda commented Jun 1, 2016

Hi

Extended2017 is the same as Extended2015 for HCAL right now which is worse that Extended2016a and surely of the two dev ones.

Sunanda


From: Ianna Osborne [notifications@github.com]
Sent: 01 June 2016 17:33
To: cms-sw/cmssw
Cc: Sunanda Banerjee; Mention
Subject: Re: [cms-sw/cmssw] Include full alignment and APE for Pixel Phase-I detector (#14545)

@kpedro88https://github.com/kpedro88 - yep, we have agreed on naming conventions, but they were not respected for 2016 scenarios:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideDetectorDescription#Adding_New_Geometry_Scenario


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/14545#issuecomment-223031407, or mute the threadhttps://github.com/notifications/unsubscribe/AEzMujYggnPe35BtRd2rnD_sYKGZ998Iks5qHaY5gaJpZM4IhD5Y.

@mmusich
Copy link
Contributor Author

mmusich commented Jun 1, 2016

All: this looks pretty much like a mess. @ianna would it be possible to create a geometry which is like Extended2016a for Hcal and Extended2017 for everything else?

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

@mmusich - I agree with both of your comments...

@ianna
Copy link
Contributor

ianna commented Jun 1, 2016

@mmusich - yes, I'll do that.

@abdoulline
Copy link

Wow...
Sorry for not participating (I had an appointment)...

When I've participated today in a round-table discussion with Yana/David/Vladimir/Michael, I had the impression it was about 2017dev (means new HCAL in 2017, not 2017 with 2015 HCAL), so I've suggested GT Geometry + hardcoded conditions for 2017dev.
And also answered to Yana's question about hardcoded->DB 2017dev conditions: under way, but no deadlines. BTW, we have had (successfully) hardcoded conditions in SLHC releases for 2019/2023, while ECAL was very busy making an re-making all the time a number of conditions in DB for various lumis...

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 1, 2016

@abdoulline - I think in general, there is a place for hardcoded (read: algorithmically generated outside of DB) conditions, when dealing with rapidly changing conditions. Aging studies are a good example, and I think that HCAL upgrade development should be another example, as we won't know the real conditions until the hardware is ready. But in this immediate case, I think we might as well avoid complicating the phase 1 pixel development if we can.

@abdoulline
Copy link

Agree.

On Wed, 1 Jun 2016, Kevin Pedro wrote:

@abdoulline - I think in general, there is a place for hardcoded (read: algorithmically
generated outside of DB) conditions, when dealing with rapidly changing conditions. Aging
studies are a good example, and I think that HCAL upgrade development should be another
example, as we won't know the real conditions until the hardware is ready. But in this
immediate case, I think we might as well avoid complicating the phase 1 pixel development if we
can.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02neJpsCsk28KCFaecp8vDPAPqUCQks5qHbYkgaJpZM4IhD5Y.gif]

@mmusich
Copy link
Contributor Author

mmusich commented Jun 2, 2016

Thanks all for the constructive feedback. Follow-up at #14748

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