HCAL M0 timing update/cleanup #26054
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26054/8605
|
|
A new Pull Request was created by @abdoulline (Salavat Abdullin) for master. It involves the following packages: RecoLocalCalo/HcalRecAlgos @cmsbuild, @perrotta, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
In the old code, there used to be a check for "negative excursions". The new code can, in principle, take the M0 time to outer space in case emax0 and emax1 have opposite signs and esum ends up close to 0. Are we really ok with that? |
|
Igor, I did saw this effect for low-E hits, when debugging.
Well, it's not that different in a sense from the default -9999f.
I thought to force M0 timing to stay in a reasonable/limited region may not
needed, as previously there were always kind of artificial "horns" at the limits
of the timing spread region.
NB: (edited) actually a check for "negative excursions" was needed previously to use tabulated correction (denoted as (i) in the intro above) , which is not used anymore.
…On Mon, 4 Mar 2019, Igor Volobouev wrote:
In the old code, there used to be a check for "negative excursions". The new code
can, in principle, take the M0 time to outer space in case emax0 and emax1 have
opposite signs and esum ends up close to 0. Are we really ok with that?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02kgqXeHcxhgnM8AkteiDghpdj1smks5vTPAvgaJpZM4bbz_2.gif]
|
|
I believe that the "negative excursions" check was needed in order to simply calculate the energy-weighted average of the three time slices. Naturally, negative energies, used as weights, make little sense in such a calculation. How about just checking whether emax1 in the new code is positive? If it is, proceed as originally intended (emax0 will be larger than or equal to emax1). If it is not, set the time to (maxI - soi)*25.f. In such a way we will also avoid potential division by 0. |
|
Initially I thought to set explicit protection against emax0+emax1 sum,
but then recalled that we never have emax0+emax1 = 0, as all channels have slightly different per-CapId characteristics (QIE calibration), so each adjacent TS has somewhat different value of energy and this sum can be small, but not 0. So I've left the change to be "minimalistic".
However, now I've realized that in some MC cases (when we use "unified" hardcode conditions, not copy of the data ones, as usual), 0 sum might happen under some circumstances. So what you've suggested sounds reasonable. Let me update the PR.
…On Mon, 4 Mar 2019, Igor Volobouev wrote:
I believe that the "negative excursions" check was needed in order to simply calculate the energy-weighted average
of the three time slices. Naturally, negative energies, used as weights, make little sense in such a calculation.
How about just checking whether emax1 in the new code is positive? If it is, proceed as originally intended (emax0
will be larger than or equal to emax1). If it is not, set the time to (maxI - soi)*25.f. In such a way we will also
avoid potential division by 0.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02lCcofTHWq-IRB3fmZQKEnTTy0scks5vTPeLgaJpZM4bbz_2.gif]
|
cmsbuild
added
code-checks-pending
tests-pending
and removed
code-checks-approved
tests-started
labels
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26054/8606
|
|
The tests are being triggered in jenkins. |
|
@cmsbuild please abort test |
|
Jenkins tests are aborted. |
|
@cmsbuild please test seems ready now |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
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, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
As discussed at HCAL DPG on Friday
https://indico.cern.ch/event/795881/
current M0 timing is suboptimal both for Phase1 collisions and for Cosmics/Splashes, as it uses
(i) tabulated corrections for old HPD pulse shape;
(ii) timing adjustment from obsolete (Run 1) DB conditions.
Both (i) and (ii) are now removed, SOI reference is used solely for collisions, not for Cosmics/Splashes. MC Cosmics-config and collisions [*] tests (including debugging printout inspection of ~100 ev) show expected behaviour: M0 timing vs energy is narrow and stable above several GeV.
[*]
NB: green color is for new M0 timing, the red one - for current default MAHI timing.
The latter is a pending issue #25526 and is being revised now.
HB :
https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/10_X/10_5_0_pre2_2018_newM0timing_vs_10_5_0_pre2_2018_SinglePi/RecHitsTask_timing_vs_energy_profile_HB.gif
HE :
https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/10_X/10_5_0_pre2_2018_newM0timing_vs_10_5_0_pre2_2018_SinglePi/RecHitsTask_timing_vs_energy_profile_HE.gif