Allowing HF anodes with special TDC values to participate in energy reco

[igv4321]

This change will allow the HF anodes with QIE10 "undershoot" and
"overshoot" TDC codes (63 and 62) to participate in the rechit energy
reconstruction. The study leading to this adjustment is described
in the following talks:

https://indico.cern.ch/event/649519/contributions/2641896/attachments/1495616/2326893/20170719_Jae_HCALPSG_TDC6263.pdf

https://indico.cern.ch/event/652743/contributions/2656951/attachments/1506347/2347457/20170809_Jae_HCALPSG_TDC6263.pdf

Here is a brief summary of issues:

-- The energy collected by the anode with "undershoot" is only weakly
correlated with the energy collected by its neighbor anode. Therefore,
discarding the anode with "undershoot" (and multiplying the energy of
the neighbor by 2) would normally lead to incorrect energy determination.

-- It is believed that the "overshoot" condition is most likely caused
by pile-up rather than by particle hitting the PMT window and
migrating the pulse to the previous time slice. The anodes with
"overshoot" do exhibit substantial correlation with their neighbors,
so it is judged that the effect of pile-up is small and can be
neglected.

This PR will not modify any relval results for MC. Minor changes are
expected for data.

[cmsbuild]

A new Pull Request was created by @igv4321 (Igor Volobouev) for master.

It involves the following packages:

RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mariadalfonso, @argiro this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pr-code-checks/PR-20170/111

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/jenkins-artifacts/pr-code-checks/PR-20170/111/git-diff.patch

In future, you can run scram build code-checks to apply code checks

[igv4321]

The code check complains about simultaneous use of "virtual" and "override" keywords -- can we skip this simple redundancy issue for now? It also wants "override" instead of "virtual" on destructors. This is crazy -- there is no way to misspell the destructor name, so "override" is meaningless there.

[perrotta]

please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22302/console Started: 2017/08/15 14:23

[cmsbuild]

+1
Tested at: 470ff69
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20170/22302/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
7d6f190
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20170/22302/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20170/22302/git-merge-result

[cmsbuild]

Comparison job queued.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20170/22302/summary.html

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 437 differences found in the comparisons
• DQMHistoTests: Total files compared: 26
• DQMHistoTests: Total histograms compared: 2718193
• DQMHistoTests: Total failures: 382
• DQMHistoTests: Total nulls: 2
• DQMHistoTests: Total successes: 2717620
• DQMHistoTests: Total skipped: 189
• DQMHistoTests: Total Missing objects: 0
• Checked 107 log files, 14 edm output root files, 26 DQM output files

[slava77]

The PR description states:

Minor changes are expected for data.

Here are some comparisons from jenkins 136.788 (red is with this PR)
Almost an order of magnitude more HF hits are reconstructed (notably, there is a drop in the number of hits at higher energies)

this propagates to higher level reco
e.g. in caloTowers, which appear with large negative times

HF hadrons count is up as well

In this case the changes are clearly not minor.
I would like to get some feedback before this can be signed off
@kpedro88 @deguio @hatakeyamak

[deguio]

@jaehyeok @abdoulline

[abdoulline]

We knew there was a lot of low-energy hits (filtered out previously, not anymore) with TDC codes 62/63 from Jae's studies. What I didn't expect - a reduction of hits in 20-40 GeV region (200-300 fC?) as shown in Slava's plots. It might be an effect of *no more second anode recovery* : now both anodes used in most of the cases, while previously it was often solely one of the two actually used (once another one came with TDC=63/63) multiplied by 2.

…

On Mon, 21 Aug 2017, Federico De Guio wrote: @jaehyeok @abdoulline — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.[AEx02u-JvxfuYPU9htKcBFss8aNXLyQpks5saW-ZgaJpZM4O3M3Z.gif]

[slava77]

We knew there was a lot of low-energy hits
...
What I didn't expect - a reduction of hits in 20-40 GeV region
...

So, If I understand correctly

• actually large changes in hit population were expected (contrary to the PR description; although it seemed clear from the slides)
• drop in population at high energy is not necessarily expected and needs more investigation.
  • If I understood the slides correctly, this is related to TDC=62 overshoot hits.

[igv4321]

Hm... I expected minor changes in comparison with the currently used 200 fC cutoff for requiring "good" TDC measurement (this PR effectively raises this cutoff to infinity). It looks like the relvals are using a global tag with an older version of "HFPhase1PMTParams" table, where that cutoff was not set.

[slava77]

In case HFPhase1PMTParams tag is wrong in the GT,
which one should be used?

the 2017 workflows in the relval matrix are using 'run2_data_promptlike' or '93X_dataRun2_PromptLike_v0'
This has
HFPhase1PMTParamsRcd with a tag HFPhase1PMTParams_v1.0_hlt
https://cms-conddb.cern.ch/cmsDbBrowser/list/Prod/gts/93X_dataRun2_PromptLike_v0

@lpernie @arunhep

[deguio]

Hi @slava77
that tag should work fine for runs after 297466 which is when it has been updated for the first time this year. the previous IOV was bugged.

is changing the run something you could do easily?
otherwise we are updating HFPhase1PMTParams_v1.0_offline for the re-reco, but it is not ready yet from what I see.

[slava77]

On 8/21/17 2:50 PM, Federico De Guio wrote: Hi @slava77 <https://github.com/slava77> that tag should work fine for runs after 297466 which is when it has been updated for the first time this year. the previous IOV was bugged.

Is there a tag of this payload that is fixed? I can try it on top of this 136.788 already posted (297227). I suppose, the alternative is to use a more recent run.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20170 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbky8XhhwsBAXHrDLba7UEwQmL_kwks5safu8gaJpZM4O3M3Z>.

[deguio]

yes. you can use: HFPhase1PMTParams_2017v01

[slava77]

it would be nice to have a GT with bugfixes for the 2017 tests in the default setup of runTheMatrix tests.

[slava77]

with HFPhase1PMTParams_2017v01 I get no changes at all in 200 events for the run 297227.
It seemed from the slides that some more frequent changes are expected

[hatakeyamak]

This seems strange indeed.

@igv4321, did you see some changes in outputs when HF hits have special code 62 and 63 and with charge above 200 fC?

[slava77]

On 8/22/17 4:34 AM, Kenichi Hatakeyama wrote: This seems strange indeed. Igor, did you see some changes in outputs when HF hits have special code 62 and 63 and with charge above 200 fC?

my test was just 200 events. How frequent are these undershoot/overshoot values (given the threshold on charge)?

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20170 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbrlVykqrvWSEnjYTfIeyOu1xxPujks5saryrgaJpZM4O3M3Z>.

[hatakeyamak]

I have to ask @jaehyeok to comment on this.

[slava77]

I also processed 200 evts from a more recent run 300517 with a default GT and this one has no differences either.
If Jae or Igor confirm that these are pretty rare, we can move on with this PR.

[slava77]

code-checks

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20170/245

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20170/245/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20170/245/git-diff.patch | patch -p1

In future, you can run scram build code-checks to apply code checks

[slava77]

@smuzaffar
about the code-checks for changes only in python: why do we apply code checks to the C++ code?

[smuzaffar]

@slava77 , we only run code-checks for c++ code. For this PR the files for which code checks were ran are
https://cmssdt.cern.ch/SDT/code-checks/PR-20170/245/changed-files-selected.log i.e RecoLocalCalo/HcalRecAlgos/src/parseHFPhase1AlgoDescription.cc

[slava77]

On 8/22/17 7:05 AM, Malik Shahzad Muzaffar wrote: @slava77 <https://github.com/slava77> , we only run code-checks for c++ code. For this PR the files for which code checks were ran are https://cmssdt.cern.ch/SDT/code-checks/PR-20170/245/changed-files-selected.log i.e RecoLocalCalo/HcalRecAlgos/src/parseHFPhase1AlgoDescription.cc

ah, right. Sorry for the noise.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20170 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbg7s-OfK1bC9yfz9lH-Batkuxw7mks5sauA9gaJpZM4O3M3Z>.

[slava77]

I also processed 200 evts from a more recent run 300517 with a default GT and this one has no differences either.

correcting myself on this: there are actually some differences (after running the comparison correctly).

[jaehyeok]

Hi Slava,

From the 50k events I looked (note that this PR is to include rechits with TDC=62/63 and charge > 200 fC),

(1) there were no rechits with TDC=63 and charge > 200 fC. I expect almost no change for TDC=63 rechits in 200 events.

(2) there were about 200 Rechits with TDC=62 and charge > 200 fC. I expect very small change for TDC=62 rechits in 200 events.

So, if there is only a minor difference (as Igor mentioned in the original message), this is expected.

[slava77]

+1

for #20170 470ff69

• allows hits with undershoot and overshoot flags in tdc; only in data
• jenkins tests pass and comparisons show no differences except in the 2017 workflow; large differences in this 2017 wf 136.788 should be ignored due to a known "bug" in the prompt-like GT
• local tests on run 300517 MuonEG PD show small differences in hfreco which propagates to small changes downstream; this is in line with small rate of changes expected as described in Allowing HF anodes with special TDC values to participate in energy reco #20170 (comment)

here are a couple of plots

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2)

[davidlange6]

+1