Fix for ECAL saturation in the tail of the pulse #20059
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 CMSSW_9_2_X. 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 |
|
please test |
|
The tests are being triggered in jenkins. |
|
@emanueledimarco Does this effect show up only in data, or is it present in MC as well? Do these saturated hits propagate at a large fraction to the egamma and PF objects to affect MET and rate of high-ET egamma events in a noticeable way? |
sure.
large majority of these cases (which is a very small fraction of data themselves) should come from spikes, which are not present in MC.
No, this should cure only rare cases happening in data. |
|
On 8/4/17 9:20 AM, Emanuele Di Marco wrote:
Do these saturated hits propagate at a large fraction to the egamma
and PF objects to affect MET and rate of high-ET egamma events in a
noticeable way?
If so, this PR may have to wait until we move to the release which
will also be used to rereco all data taken so far (93X).
The policy is to keep the physics performance consistent in a
production release.
No, this should cure only rare cases happening in data.
Does this "no" mean that in events with such hits the hits typically do
not propagate to
egamma and PF objects ?
Perhaps if there is a skim of events with the hits affected by this
change can be shared for inspection of the impact of this PR on
downstream reco objects.
If this PR is for rare cases that do not affect physics behaviour of
reconstructed objects,
then we just better stay with 93X version.
|
the rechit was not properly flagged as kSaturated, so that rechit was propagated to the clusters as a normal one (with a wrong amplitude)
I have not a proper skim, but 3 picked events from @gennai in this file that contain a problematic rechit:
Yes, it addresses rare cases happening almost only in data. So OK for 93X-only. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
1 similar comment
|
+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 @emanueledimarco |
|
Yes, it is OK to apply this fix only in 93X. |
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