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-hcx59New Expand Hcal DetId replacing Run2-hcx29 (11427), Run2-hcx48 (12406) and Run2-hcx58 (12726) #12829

Closed
wants to merge 2 commits into from

Conversation

bsunanda
Copy link
Contributor

Trying to use data model evolution capability for HcalDetId

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @bsunanda 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

@bsunanda
Copy link
Contributor Author

@cmsbuild Please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@@ -6,6 +6,7 @@
#include "Geometry/Records/interface/HcalRecNumberingRecord.h"

#include <iostream>
using namespace std;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be removed

@slava77
Copy link
Contributor

slava77 commented Dec 21, 2015

-1

Things to be followed up:

  • digitizer couts shouuld be cleaned up
  • "using namespace" in a header file
  • reading of old SIM for mixing appear to be broken. There are very large differences in jenkins comparisons of workflows with pileup. E.g. in 50202 in jenkins comparisons
    all_oldvsnew_ttbarpuwf50202p0c_calotowerssorted_towermaker__reco_obj_obj_eta

Also,

  • HIJetValidation plots have small changes in 2011 data processing (wf 140.53)
    e.g.
    wf140 53_ak4pupf_phi
    this may be unrelated to the format change.
    Still, something to check next time an update is made.

@bsunanda
Copy link
Contributor Author

@cmsbuild Please test
@slava77 @kpedro88 I see the SimHits get the right format at the Digitizer i/p. So I cannot guess what could be wrong

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Dec 30, 2015

@bsunanda please clarify which tests you have run to do the check at the digitizer?
Did you use the setup in jenkins from a sample with pileup (new hard scatter SIM + old minibias SIM to mix)?

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@bsunanda
Copy link
Contributor Author

@slava77 I looked at 25.0_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT. I shall look now in 25202. Thanks


From: Slava Krutelyov [notifications@github.com]
Sent: 30 December 2015 22:40
To: cms-sw/cmssw
Cc: Sunanda Banerjee
Subject: Re: [cmssw] bsunanda:Run2-hcx59New Expand Hcal DetId replacing Run2-hcx29 (11427), Run2-hcx48 (12406) and Run2-hcx58 (12726) (#12829)

I see something similar to the previous iteration
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_0_X_2015-12-27-0900+12829/11901/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/CaloTowersD_CaloTowersTask.html

This may all be coming from HF (that's unless the plotter HcalDigiMonitor is buggy itself)
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_0_X_2015-12-27-0900+12829/11901/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/Hcal_DigiMonitor_Hcal_digi_info_HF.html


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

@slava77
Copy link
Contributor

slava77 commented Dec 30, 2015

The issue is visible only in workflows with pileup. 25.0 doesn't have any.

@bsunanda
Copy link
Contributor Author

@slava77 I tried with 25202 and there too I saw all hits input to HcalDigitizer have new format. In any case if there is some with old ones they can be changed to new format in the digitizaer code.


From: Slava Krutelyov [notifications@github.com]
Sent: 30 December 2015 23:02
To: cms-sw/cmssw
Cc: Sunanda Banerjee
Subject: Re: [cmssw] bsunanda:Run2-hcx59New Expand Hcal DetId replacing Run2-hcx29 (11427), Run2-hcx48 (12406) and Run2-hcx58 (12726) (#12829)

The issue is visible only in workflows with pileup. 25.0 doesn't have any.


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

@slava77
Copy link
Contributor

slava77 commented Dec 31, 2015

On 12/31/15 4:58 AM, bsunanda wrote:

@slava77 I tried with 25202 and there too I saw all hits input to
HcalDigitizer have new format. In any case if there is some with old
ones they can be changed to new format in the digitizaer code.

Just to be sure, were they the same for HF?


From: Slava Krutelyov [notifications@github.com]
Sent: 30 December 2015 23:02
To: cms-sw/cmssw
Cc: Sunanda Banerjee
Subject: Re: [cmssw] bsunanda:Run2-hcx59New Expand Hcal DetId replacing
Run2-hcx29 (11427), Run2-hcx48 (12406) and Run2-hcx58 (12726) (#12829)

The issue is visible only in workflows with pileup. 25.0 doesn't have any.


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


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

@bsunanda
Copy link
Contributor Author

Yes it is. It is for all hcal subdetectors: hb, he, ho and hf

Sent from Samsung Mobile

Slava Krutelyov notifications@github.com wrote:
On 12/31/15 4:58 AM, bsunanda wrote:

@slava77 I tried with 25202 and there too I saw all hits input to
HcalDigitizer have new format. In any case if there is some with old
ones they can be changed to new format in the digitizaer code.

Just to be sure, were they the same for HF?


From: Slava Krutelyov [notifications@github.com]
Sent: 30 December 2015 23:02
To: cms-sw/cmssw
Cc: Sunanda Banerjee
Subject: Re: [cmssw] bsunanda:Run2-hcx59New Expand Hcal DetId replacing
Run2-hcx29 (11427), Run2-hcx48 (12406) and Run2-hcx58 (12726) (#12829)

The issue is visible only in workflows with pileup. 25.0 doesn't have any.


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


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


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

@slava77
Copy link
Contributor

slava77 commented Dec 31, 2015

@bsunanda
I looked at HFDataFramesSorted_simHcalDigis__HLT in 25202 step2 outputs from jenkins and plotted interactively HFDataFramesSorted_simHcalDigis__HLT.obj.obj.data_.adc() for adc()!=0: the baseline has adc() average of 17.4, while with this PR the mean is 3.4.
This drop seems consistent with the drop in towers.
This suggests to me that the problem is in the digitizer/mixing.
Even if the format is the same, according to your observations, something is breaking the outputs.

@slava77
Copy link
Contributor

slava77 commented Dec 31, 2015

here come merge conflicts

@kpedro88
Copy link
Contributor

kpedro88 commented Jan 6, 2016

@slava77 - I made a new PR that hopefully resolves the problems. See #12883.

@slava77
Copy link
Contributor

slava77 commented Jan 7, 2016

-1
superseded

@davidlange6 davidlange6 closed this Jan 7, 2016
davidlange6 added a commit that referenced this pull request Jan 12, 2016
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

6 participants