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

HBHE: add photostatitics uncertainty in the M2 #17533

Merged
merged 1 commit into from Feb 20, 2017

Conversation

mariadalfonso
Copy link
Contributor

This PR is a follow up on the #17311
And make use of the new GT #17530

The noise uncertainty now is added in the M2 chi2 also for the HPD.
validation slides are here
http://dalfonso.web.cern.ch/dalfonso/PhotoStatHPD.pdf

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Configuration/AlCa
RecoLocalCalo/HcalRecAlgos

@ghellwig, @cvuosalo, @arunhep, @cerminar, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @ghellwig, @tocheng, @argiro 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

@slava77
Copy link
Contributor

slava77 commented Feb 16, 2017

@cmsbuild please test

there appear to be run time issues with #17530
Also, #17530 GT updates geometry in 2017, which makes this PR change incrementally untestable.

So, this PR may not move until #17530 is merged.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 16, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17814/console Started: 2017/02/16 13:31

@cmsbuild
Copy link
Contributor

-1

Tested at: 2dae29d

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17533/17814/summary.html

I found follow errors while testing this PR

Failed tests: AddOn

  • AddOn:

I found errors in the following addon tests:

cmsDriver.py TTbar_Tauola_8TeV_TuneCUETP8M1_cfi -s GEN,SIM,DIGI,L1,DIGI2RAW --mc --scenario=pp -n 10 --conditions auto:run1_mc_Fake --relval 9000,50 --datatier "GEN-SIM-RAW" --eventcontent RAWSIM --customise=HLTrigger/Configuration/CustomConfigs.L1T --fileout file:RelVal_Raw_Fake_MC.root : FAILED - time: date Thu Feb 16 14:52:48 2017-date Thu Feb 16 14:48:35 2017 s - exit: 34304
cmsRun /cvmfs/cms-ib.cern.ch/nweek-02459/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_0_X_2017-02-15-2300/src/HLTrigger/Configuration/test/OnLine_HLT_Fake.py realData=False globalTag=@ inputFiles=@ : FAILED - time: date Thu Feb 16 14:52:48 2017-date Thu Feb 16 14:48:35 2017 s - exit: 21504
cmsDriver.py RelVal -s HLT:Fake,RAW2DIGI,L1Reco,RECO --mc --scenario=pp -n 10 --conditions auto:run1_mc_Fake --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --processName=HLTRECO --filein file:RelVal_Raw_Fake_MC.root --fileout file:RelVal_Raw_Fake_MC_HLT_RECO.root : FAILED - time: date Thu Feb 16 14:52:48 2017-date Thu Feb 16 14:48:35 2017 s - exit: 21504

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@franzoni
Copy link

Alca signature subject to resolution of #17530
Where investigations are on going re content of payloads

@abdoulline
Copy link

abdoulline commented Feb 17, 2017

I've fixed the content of HcalSiPMParameters for Run 1, Walter will submit a new tag (HcalSiPMParameters_v1.2_mc) today. Plus two data ones with updated Run 1 IOVs.
Sorry about this Run 1 conditions mistake, which slows down the pace of updates...


noisePHArr[ip] = sqrt((charge-ped)*0.3305);
if(!channelData.hasTimeInfo() && (charge-ped)>noise_) {
noisePHArr[ip] = sqrt((charge-ped)*channelData.fcByPE());
Copy link
Contributor

Choose a reason for hiding this comment

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

is the value here consistent with 0.3305 used in PulseShapeFitOOTPileupCorrection::apply?
I was actually expecting that the same source is used in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with the new GT, for HPD, printing on the screen the channelData.fcByPE() is 0.3305

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thank you for the confirmation.
Can we avoid the hardcoded value in the PulseShapeFitOOTPileupCorrection::apply and get it from DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the amazing class HBHEChannelInfo channelData.fcByPE() is only available for the Phase1Code.

For the run1/run2 code I will likely have to do a lot of gymnastic.
I tried to do this in the past unsuccessfully for the other parameters i.e. the noise mariadalfonso@c923f24
The old M2 code is anyway full of all hard coded numbers so one more or less should not make a big difference. I can see if I can find an easy way, but no promises (maybe we can put in the post-pre5 SW rework)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@igv4321 please clarify what is the status of moving all of the hbhe reco to the phase-1 code?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are working on it. Approximate time line when it happens is end of Feb. @halilg might be able to provide more details.

@slava77
Copy link
Contributor

slava77 commented Feb 17, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Feb 19, 2017

there is a merge conflict here now

@cmsbuild
Copy link
Contributor

Pull request #17533 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again.

@mariadalfonso
Copy link
Contributor Author

@slava77
rebased on top of CMSSW_9_0_X_2017-02-19-1100

@slava77
Copy link
Contributor

slava77 commented Feb 19, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 19, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17871/console Started: 2017/02/19 20:39

@slava77
Copy link
Contributor

slava77 commented Feb 19, 2017

[the following is based on https://github.com//pull/17533 2dae29d; made in CMSSW_9_0_X_2017-02-16-1100 as a baseline; I made a quick check with the later version to confirm that the conclusions do not change]

The main change from this PR is improved chi2 in hbhe hits
10224:
barrel
all_sign860vsorig_ttbar13tevpu2017wf10224p0c_log10hbherechitssorted_hbhereco__reco_obj_obj_chi210

endcap
all_sign860vsorig_ttbar13tevpu2017wf10224p0c_log10hbherechitssorted_hbhereco__reco_obj_obj_chi220

There is also a significant reduction (by x2 in the barrel) in the number of hits with the 3-pulse fit flag (HBHEPulseFitBit=29)

The fraction of non-zero (E>1 MeV) hits is up in the PU sample:
by 7% in the barrel
all_sign860vsorig_ttbar13tevpu2017wf10224p0c_log10hbherechitssorted_hbhereco__reco_obj_obj_energy3

and by 13% in the endcaps
all_sign860vsorig_ttbar13tevpu2017wf10224p0c_log10hbherechitssorted_hbhereco__reco_obj_obj_energy13
Since the 1-pulse fit is tried first, we should expect more OOT PU energy to come in for the fraction of previously 3-pulse fitted hits, which OTOH also had a fraction of in-time energy "eaten" by the OOT component.

The number of neutral hadrons is up by 12%
all_sign860vsorig_ttbar13tevpu2017wf10224p0c_log10recopfcandidates_particleflow__reco_obj_pt73

This transfers to probably 1-2% increase in lower-pt jet response
wf10224_ak4calo_corovergen_eta_e

Jet samples without pileup show smaller differences in response, with sub-% increase in the TeV range.
Time of rechits is more flat now [wf 10071 flat-pt QCD sample]
wf10071_hittime_vs_e_hb
wf10071_hittime_vs_e_he

chi2 is smaller, as expected
wf10071_log10chi2_vs_e_hb
wf10071_log10chi2_vs_e_he

In data, changes look fairly similar to ttbar MC with PU
e.g. in 2016E JetHT (136.761)
chi2 in endcap
all_sign860vsorig_runjetht2016ewf136p761c_log10hbherechitssorted_hbhereco__rereco_obj_obj_chi220
hit energy E>1MeV in endcap
all_sign860vsorig_runjetht2016ewf136p761c_log10hbherechitssorted_hbhereco__rereco_obj_obj_energy13

On the technical side, in PU35 ttbar (wf10224)

  • the hbheprereco time is down by 24% (or about 1% of total reco time)
  • given the increase in the number of hits with E>0 some module times are up:
    • particleFlowClusterHBHE up by 50% (from 7 to 11 ms/evt)
    • particleFlowClusterHCAL up by 15% (negligible for the total; under 2ms/evt)

Overall, this seems ready to sign; I'll wait for jenkins tests to finish.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Feb 19, 2017

+1

for #17533 fd3b827

  • changes are as described: to add the photostatistics component to the error used in hit fit minimization in M2; affects all simulations with HPDs
  • jenkins tests pass and comparisons with baseline show differences that start from hbhe hit reco and propagate downstream; differences are consistent with what's found in the local tests summarized in HBHE: add photostatitics uncertainty in the M2 #17533 (comment)
    • the tests after rebase also do not show any irrelevant differences as present earlier in the comparisons due to the GT changes that were included with this PR

@cmsbuild
Copy link
Contributor

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 b85c807 into cms-sw:CMSSW_9_0_X Feb 20, 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

7 participants