Fix for ECAL saturation in the tail of the pulse #20060
Conversation
…ing multifit or not. This do not consider cases with saturation on the tail
|
A new Pull Request was created by @emanueledimarco (Emanuele Di Marco) for master. It involves the following packages: RecoLocalCalo/EcalRecProducers @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| result.emplace_back((*itdg).id(), 4095*12, 0, 0, 0); | ||
| auto & uncalibRecHit = result.back(); | ||
| uncalibRecHit.setFlagBit( EcalUncalibratedRecHit::kSaturated ); | ||
| // do not propagate the default chi2 = -1 value to the calib rechit (mapped to 64), set it to 0 when saturation | ||
| uncalibRecHit.setChi2(0); | ||
| } else if ( leadingSample >= 0 ) { // saturation on other samples: cannot extrapolate from the fourth one | ||
| } else if ( lastSampleBeforeSaturation >= 0 ) { // saturation on other samples: cannot extrapolate from the fourth one |
There was a problem hiding this comment.
As such, if you have saturation on the first sample (lastSampleBeforeSaturation = -1) you don mark the uncalibRecHit as kSaturated and go multifit as well: is this the wanted behavior? (Easier to ask you instead of trying to understand the full logic behind...)
|
@emanueledimarco (if you are not in vacation in these days) : any news about #20060 (review) ? |
|
@bendavid @argiro : assuming @emanueledimarco has left for vacations, let try if someone is still around these days and can answer my doubt in #20060 (comment) ... |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
@perrotta thanks for spotting this. It is a bug. Also if sample 0 is saturated the rechit should be flagged as saturated. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20060/214 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-20060/214/git-diff.patch In future, you can run |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
On 8/21/17 7:44 AM, Emanuele Di Marco wrote:
@slava77 <https://github.com/slava77> I am not aware of this plan. So
addressed it here for EcalUncalibRecHitWorkerMultiFit.cc
header files were checked as well and the check has also a diff for a
patch in
EcalUncalibRecHitWorkerMultiFit.h
Please change this as well if it's OK.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20060 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbpbNJFMgxLceEWTzw_nZPFMG7ESVks5saZexgaJpZM4Ot74U>.
|
|
Comparison is ready Comparison Summary:
|
|
yes, it's OK. Done ( thanks @slava77 ). |
|
+code-checks |
|
@cmsbuild please test @emanueledimarco thank you |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
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 (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
This PR fixes the handling of cases where the saturation was happening in the tail of the pulse shape, instead of the neighbours of the max sample.
In these cases, due to a wrong behavior of EcalDataFrame.lastUnsaturatedSample(), if the saturation happened in the last samples of the tail:
With this PR the multifit is not called if any of the sample is saturated, and the last saturated sample is set to the one before the first happening of the saturation.
This fixes the occurrences of the warnings seen by @gennai at HLT.
In the meanwhile we investigate within ECAL about the implementation of EcalDataFrame.lastUnsaturatedSample() and eventually will fix it (or remove, since it is not used elsewhere in CMSSW).
@bendavid @amassiro @crovelli @paramatti
Same change as done for the 92X branch, in PR #20059