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

bsunanda:Run2-hcx101 Make geometry change to accommodate Plan 1 operation of HCAL #17330

Merged
merged 2 commits into from Feb 7, 2017

Conversation

bsunanda
Copy link
Contributor

No description provided.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @bsunanda for CMSSW_9_0_X.

It involves the following packages:

Configuration/Geometry
Geometry/CMSCommonData
Geometry/CaloTopology
Geometry/HcalCommonData
Geometry/HcalTowerAlgo
RecoLocalCalo/CaloTowersCreator
SimCalorimetry/HcalSimAlgos
SimCalorimetry/HcalSimProducers
SimGeneral/CaloAnalysis
Validation/HcalHits

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

cms-bot commands are listed here #13028

@bsunanda
Copy link
Contributor Author

@cmsbuild Please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 30, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17516/console Started: 2017/01/30 23:51

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Copy link
Contributor

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@bsunanda - updated 2017 base scenario is in CMSSW_9_0_X branch. It should be available in the next IB.


## 2015 + new phase 1 pixel detector

XMLIdealGeometryESSource = cms.ESSource("XMLIdealGeometryESSource",
Copy link
Contributor

Choose a reason for hiding this comment

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

@bsunanda - is it yet another HCal scenario? Please, make sure it is synchronised to current 2017 and 2018 ones (e.g. GEM slice + overlap fixes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is yet another scenario for HCAL so called Plan-1 where only one RBX of HE is upgraded. The current 2017 scenario is so called Plan-0 where only HF is upgraded and HE is not. The current 2018 scenario is Plan-36 when all 36 RBX's of HE are upgraded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you, please, make sure that this new scenario differs from the default 2017 only in HCal? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does that. I started with 2017 scenario and changed the HCAL part only

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

please, check it against recent IB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

@@ -265,7 +265,7 @@ void CaloTowersCreationAlgo::setGeometry(const CaloTowerTopology* cttopo, const
std::vector<int> tower28depths;
int ndepths, startdepth;
theHcalTopology->getDepthSegmentation(theHcalTopology->lastHERing()-1,tower28depths);
theHcalTopology->depthBinInformation(HcalEndcap,theHcalTopology->lastHERing()-1,ndepths,startdepth);
theHcalTopology->depthBinInformation(HcalEndcap,theHcalTopology->lastHERing()-1,1,1,ndepths,startdepth);
Copy link
Contributor

Choose a reason for hiding this comment

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

where is the "plan1" wedge?
.. in case 1,1 is the plan-1 wedge and this happens to be a full phase-1 depth configuration (not hits merged to look like phase-0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a partial work - just make the code compile. It needs further work. Should I take care of this. I thought it is better the Plan-1 version is to be integrated and then we clean up CaloTower, Validation and DQM code needed for this.

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 fine for a work in progress.

Still, where is the "plan1" wedge in units of iphi and zside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is z= +1 and phi = 63..66. I am looking into possible changes in the CaloTowerCreationAlgo - we have to introduce zside, phi dependence there

@bsunanda
Copy link
Contributor Author

bsunanda commented Feb 2, 2017

@slava77 @ianna @civanch Could you kindly approve this so that we can move forward

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2017

Pull request #17330 was updated. @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @dmitrijus, @kpedro88, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@bsunanda
Copy link
Contributor Author

bsunanda commented Feb 2, 2017

@kpedro88 @dmitrijus Please take a look and approve this

@bsunanda
Copy link
Contributor Author

bsunanda commented Feb 2, 2017

The next set of changes can follow after this

@kpedro88
Copy link
Contributor

kpedro88 commented Feb 2, 2017

+1

@davidlange6
Copy link
Contributor

davidlange6 commented Feb 3, 2017 via email

@abdoulline
Copy link

abdoulline commented Feb 3, 2017 via email

@kpedro88
Copy link
Contributor

kpedro88 commented Feb 3, 2017

Yes, unfortunately it seems like we'll have to support separate plan0 and plan1 scenarios officially for a while. Once this PR and #17313 are merged, we can try to start putting together the plan1 scenario. JetMET is asking for it on a more or less hourly basis.

@davidlange6
Copy link
Contributor

davidlange6 commented Feb 3, 2017 via email

@ianna
Copy link
Contributor

ianna commented Feb 3, 2017

@bsunanda - there is no place in DB for Plan1 now. If we have to support both Plan0 and Plan1 we'll need a new GT queue unless HCal re-writes the code for conditions and reco geometry to have labels.

@franzoni - FYI

@kpedro88
Copy link
Contributor

kpedro88 commented Feb 3, 2017

If we're going to change the name of 2017dev to 2017Plan1, it would be nice to add it to GeometryConf.py also

@bsunanda
Copy link
Contributor Author

bsunanda commented Feb 3, 2017

I shall do that in the next PR - let this be integrated first

@davidlange6
Copy link
Contributor

its a git mv then git push no? it should be straightforward to have it here and avoid the history messiness

@slava77
Copy link
Contributor

slava77 commented Feb 3, 2017

+1

for #17330 7c55342

  • reco part of the changes is rather technical, to update to the new interface of HcalTopology::depthBinInformation
  • jenkins tests pass and comparisons show no differences

@bsunanda
Copy link
Contributor Author

bsunanda commented Feb 4, 2017

@dmitrijus Please approve this at the earliest
@davidlange6 Kindly merge this at the earliest so that I can add next set of changes to this

@bsunanda
Copy link
Contributor Author

bsunanda commented Feb 6, 2017

@davidlange6 Could you merge this at the earliest - it stops me to go to the next step

@abdoulline
Copy link

abdoulline commented Feb 6, 2017 via email

@davidlange6
Copy link
Contributor

so far my request is even unanswered...we've a mix of 2017dev 2017a 17b blah blah. Lets fix a naming convention and get it in this PR - I think people were happy with 2017Plan1

@bsunanda
Copy link
Contributor Author

bsunanda commented Feb 6, 2017

I shall rename the scenario as 2017Plan1 in the next PR

@davidlange6
Copy link
Contributor

davidlange6 commented Feb 6, 2017 via email

@bsunanda
Copy link
Contributor Author

bsunanda commented Feb 6, 2017

@davidlange6 Please let it go - we shall miss the timeline for participation in global run otherwise

@dmitrijus
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2017

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 6e950fa into cms-sw:CMSSW_9_0_X Feb 7, 2017
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