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

merge SLHC updates for CaloTowers #11497

Merged
merged 18 commits into from Oct 20, 2015
Merged

Conversation

kpedro88
Copy link
Contributor

This PR updates the CaloTower geometry, topology, and algorithms to be consistent with the new HCAL geometry and topology code (followup on #10353). (Most of these changes are ported from 62XSLHC.)

A new parameter for calotowermaker is propagated to all the relevant HLT configs.

A few notes:

  1. I already rebased this onto bsunanda:Run2-hcx28 Make the topology tester up-to-date with the geometry changes for HCAL #11423, to avoid a future (small) merge conflict.
  2. The matrix tests will crash. This is expected because the new CaloTower geometry record is not in the database yet.
  3. I validated this PR using 4.53 with the XML geometry, comparing clean CMSSW_7_6_X_2015-09-23-2300 to the same IB with this PR (using checkdeps). The output is identical, as expected; see plots below:

eta
phi
pt

attn: @bsunanda, @ianna

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @kpedro88 (Kevin Pedro) for CMSSW_7_6_X.

merge SLHC updates for CaloTowers

It involves the following packages:

CondTools/Geometry
DQM/DataScouting
DataFormats/CaloTowers
Geometry/CaloEventSetup
Geometry/CaloGeometry
Geometry/CaloTopology
Geometry/EcalTestBeam
Geometry/HcalEventSetup
Geometry/HcalTowerAlgo
HLTrigger/Configuration
HLTrigger/HLTanalyzers
HLTrigger/JetMET
RecoEgamma/EgammaIsolationAlgos
RecoLocalCalo/CaloTowersCreator
RecoLocalCalo/HcalRecProducers
RecoParticleFlow/PFClusterProducer
RecoTauTag/RecoTau
SimDataFormats/EcalTestBeam

@perrotta, @cmsbuild, @civanch, @Dr15Jones, @cvuosalo, @fwyzard, @ianna, @mdhildreth, @Martin-Grunewald, @deguio, @slava77, @ggovi, @vanbesien, @danduggan can you please review it and eventually sign? Thanks.
@ghellwig, @apfeiffer1, @Sam-Harper, @mmarionncern, @argiro, @Martin-Grunewald, @jalimena, @lgray, @geoff-smith, @bachtis 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.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@slava77
Copy link
Contributor

slava77 commented Sep 25, 2015

changes in HLT table dump files are an instant "-1"
A fillDescriptions or a customize function need to be created

@kpedro88
Copy link
Contributor Author

Hi Slava,

I made these changes because CaloTowersCreator has a new tracked parameter. The HLT python files make the config for calotowermaker manually, so the change needs to be propagated to all of them individually. If there is a better way to propagate this update, please let me know and I will update the PR.

@slava77
Copy link
Contributor

slava77 commented Sep 25, 2015

Hi Kevin,
The best is to add fillDescriptions method to CaloTowersCreator and pass the new parameter there as a default.
A less desired alternative is HLTrigger/Configuration/python/customizeHLTforCMSSW.py

@kpedro88
Copy link
Contributor Author

Hi Slava,
I'm not sure if fillDescriptions would work well in this case. Some of the HLT towermaker configs use different values for certain parameters (thresholds, etc.), and (if I understand correctly) those would have to be customized somewhere if fillDescriptions provided some set of default values.

I opted for the less obtrusive solution, setting a default value for HcalPhase if it's not present in the parameter set. The parameter is only used for upgrade studies, so it will never need to be set explicitly for Run 2.

@cmsbuild
Copy link
Contributor

Pull request #11497 was updated. @perrotta, @cmsbuild, @civanch, @Dr15Jones, @cvuosalo, @fwyzard, @ianna, @mdhildreth, @Martin-Grunewald, @slava77, @ggovi can you please check and sign again.

@ianna
Copy link
Contributor

ianna commented Sep 26, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

}
else
{
if( isZDC() )
Copy link
Contributor

Choose a reason for hiding this comment

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

@kpedro88 - is removing zdc here intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ianna - it looks like this was lost in the rebase. I'll fix it, thanks for the catch. (I'll also rewrite this part to use "else if" instead of nested if-else sets.)

@kpedro88
Copy link
Contributor Author

These errors don't make sense to me:

An exception of category 'ConfigFileReadError' occurred while
   [0] Processing the python configuration file named /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-19-1100/src/TopQuarkAnalysis/TopEventProducers/test/tqaf_cfg.py
Exception Message:
python encountered the error: <type 'exceptions.ImportError'>
No module named CaloTowerTopology_cfi

CaloTowerTopology_cfi clearly exists in this PR. When I run that test locally, it works fine, and if I load the config interactively, I can see that it grabs the proper info from CaloTowerTopology_cfi:

python -i TopQuarkAnalysis/TopEventProducers/test/tqaf_cfg.py
>>> process.CaloTowerTopologyEP 
cms.ESProducer("CaloTowerTopologyEP")

Maybe it's just a spurious failure? It's never been a problem before. Let's try rerunning the tests...

@slava77
Copy link
Contributor

slava77 commented Oct 20, 2015

@cmsbuild please test
[unclear why there would be a glitch]

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Oct 20, 2015

+1

for #11497 09ad091

  • changes in the code look OK
  • jenkins tests almost passed here, except for two odd errors in the unit tests; the equivalent 80X PR passed in full
  • additionally tested locally in CMSSW_7_6_X_2015-10-19-1100 including tests with higher statistics: they confirm no differences in monitored quantities
    • the unit tests have passed locally in full

@ianna
Copy link
Contributor

ianna commented Oct 20, 2015

+1

@cmsbuild
Copy link
Contributor

@mmusich
Copy link
Contributor

mmusich commented Oct 20, 2015

+1
for kpedro88@d05559e

@cmsbuild
Copy link
Contributor

@civanch
Copy link
Contributor

civanch commented Oct 20, 2015

+1

davidlange6 added a commit that referenced this pull request Oct 20, 2015
merge SLHC updates for CaloTowers
@davidlange6 davidlange6 merged commit a9d34ed into cms-sw:CMSSW_7_6_X Oct 20, 2015
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