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

Expand Hcal DetId (supersedes #12829) #12883

Merged
merged 3 commits into from Jan 12, 2016

Conversation

kpedro88
Copy link
Contributor

@kpedro88 kpedro88 commented Jan 6, 2016

This (hopefully) fixes the pileup problem observed in #12829 by enforcing the new form of the HcalDetId consistently in HcalDigitizer and the HcalDetId constructor. Also, merge conflicts were resolved.

@slava77 @bsunanda - can you start the tests?

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2016

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

It involves the following packages:

CalibCalorimetry/CaloMiscalibTools
CalibCalorimetry/CastorCalib
CalibCalorimetry/HcalPlugins
DataFormats/HcalDetId
Geometry/CaloTopology
Geometry/HcalTowerAlgo
RecoHI/HiJetAlgos
RecoParticleFlow/PFClusterProducer
SimCalorimetry/HcalSimProducers
Validation/GlobalRecHits

@civanch, @diguida, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @franzoni, @Dr15Jones, @cerminar, @deguio, @slava77, @mmusich, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @mmarionncern, @yslai, @tocheng, @lgray, @richard-cms, @yenjie, @jazzitup, @kurtejung, @istaslis, @mandrenguyen, @dgulhan, @bachtis, @yetkinyilmaz this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@slava77
Copy link
Contributor

slava77 commented Jan 6, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2016

@slava77
Copy link
Contributor

slava77 commented Jan 7, 2016

+1

for #12883 6c68b62

  • changes in the code are OK;
    • note that this update includes essentially a bugfix in ParticleTowerProducer: the old code was creating CaloTowers with HcalDetId and as a result used HCAL cell positions to place the towers; the updated version uses a more appropriate CaloTowerDetId construction. The positions are not exactly the same and also the ordering of towers ends up being different (both can trigger small differences in PFTower-related objects).
  • jenkins tests pass and comparisons with baseline
    • show no differences in fwlite-based comparisons
    • show no differences except for some small differences in 140.53 (HI) workflow in HIJetValidation plots akPu*. These akPu* jets are made out of PFTowers and they are expected to change slightly.

@ianna
Copy link
Contributor

ianna commented Jan 11, 2016

+1

@civanch
Copy link
Contributor

civanch commented Jan 12, 2016

+1
@kpedro88, there are problems in FastSim WF - 5.1, 135.4

@mmusich
Copy link
Contributor

mmusich commented Jan 12, 2016

+1

@kpedro88
Copy link
Contributor Author

@civanch - Thanks, I will look into it and submit a subsequent PR when I figure out the fix.

@kpedro88
Copy link
Contributor Author

@civanch - actually, I noticed that you also see this problem in #12888, so maybe it is not caused by this PR?

@kpedro88
Copy link
Contributor Author

@civanch - Just to confirm, when I look back at older comparisons from previous PRs (even going back into December), I see the same behavior. I think the problem must have been introduced a while ago.

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