HBHE: siPM M2 reco fixes #15959
HBHE: siPM M2 reco fixes #15959
Conversation
|
A new Pull Request was created by @mariadalfonso for CMSSW_8_1_X. It involves the following packages: RecoLocalCalo/HcalRecAlgos @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
| @@ -270,7 +270,8 @@ void PulseShapeFitOOTPileupCorrection::setPUParams(bool iPedestalConstraint, b | |||
| } | |||
|
|
|||
| void PulseShapeFitOOTPileupCorrection::setPulseShapeTemplate(const HcalPulseShapes::Shape& ps, bool isHPD) { | |||
| if( cntsetPulseShape ) return; | |||
| // comment this otherwise he siPM is not correctly initialized | |||
| // if( cntsetPulseShape ) return; | |||
There was a problem hiding this comment.
does it affect the HPD/QIE8 version?
There was a problem hiding this comment.
In practice the pulse template was set only on the first rechit as they were all the same.
Now is going to be initialized for every rec-hit and properly if it belong to HPD/QIE8 or the siPM/QIE10.
So in practice this line will not have any single difference on the output, I haven't checked the CPU time on the 2016.
There was a problem hiding this comment.
May I suggest to edit this comment to clarify, smth like
// initialize for every hit now to avoid incorrect settings for different channel types (HPD vs SiPM)
// FIXME: keep this as a reminder to improve and reduce CPU use
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
@mariadalfonso |
| @@ -11,7 +11,7 @@ | |||
| ts4Max = cms.vdouble(100.,70000.), #fC # this is roughly 20 GeV | |||
| pulseJitter = cms.double(1.), #GeV/bin | |||
| ### | |||
| meanTime = cms.double(0.), #ns | |||
| meanTime = cms.double(2.), #ns | |||
There was a problem hiding this comment.
does this match better what we have in data in 2016?
There was a problem hiding this comment.
@slava77
I added one slide on the M2 timing jetHT data 2016
https://indico.cern.ch/event/570231/contributions/2306435/attachments/1341696/2021200/PR15959.pdf
Data and MC just behave differently.
So the data are aligned such as the M2 timing is at zero, when moving the initial parameter of the fit, the fitted time goes up.
The MC instead prefer the 2ns.
So probably the best things for now would be not to move this parameter and tackle this time shift difference in MC at a more fundamental level (if it can be done).
Let me know if I should remove this commit c847539 from this PR.
There was a problem hiding this comment.
Was this change already discussed and well understood within HCAL DPG, more specifically, in context of its effect on the 2016 data (and possible impact on downstream, as PF and JME)?
e.g., from jenkins tests in SinglePhoton wf 136.731:
the energy of hits goes up a bit on average
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-09-22-0900+15959/16018/validateJR/all_OldVSNew_RunSinglePh2016Bwf136p731/all_OldVSNew_RunSinglePh2016Bwf136p731c_log10HBHERecHitsSorted_hbhereco__reRECO_obj_obj_energy.png
it propagates to higher level quantities
like caloTowers
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-09-22-0900+15959/16018/validateJR/all_OldVSNew_RunSinglePh2016Bwf136p731/all_OldVSNew_RunSinglePh2016Bwf136p731c_log10CaloTowersSorted_towerMaker__reRECO_obj_obj_energy151.png
pf neutral hadron yields are up by ~4%
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-09-22-0900+15959/16018/validateJR/all_OldVSNew_RunSinglePh2016Bwf136p731/all_OldVSNew_RunSinglePh2016Bwf136p731c_log10recoPFCandidates_particleFlow__reRECO_obj_pt55.png
from the increase in jet yield by about 1%, I can guess there is sub-% (O(0.1%)?) level average increase in response ... unclear if it needs recalibration anywhere
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-09-22-0900+15959/16018/validateJR/all_OldVSNew_RunSinglePh2016Bwf136p731/all_OldVSNew_RunSinglePh2016Bwf136p731c_recoPFJets_ak4PFJets__reRECO_obj_eta.png
The change in the time distribution itself is much more obvious, but I was told in the past that it is a rather secondary variable that by itself does not contribute directly to common physics observables
There was a problem hiding this comment.
@slava77
c847539 is a different initialization of the M2-fit meant to make the fit easier to converge and less CPU rather than introducing energy performance changes.
This is what I learned from the plots I made on the single MC pion.
This specific change was only discussed in the context of the phase1 thursday meetings (twice).
In the context of the 2016, at the DPG we discussed the MC timing and a current shift the digitization of the MC back to the center but the focus was the shift of the MC simulation (so the input of the M2) https://indico.cern.ch/event/561059/contributions/2269081/attachments/1320863/1980774/2016-08-05-Miao_HCAL_meeting_updata.pdf.
In general we should bring the MC closer to data and not the other way around so best will to change the simulation rather than the reconstruction.
Another option can be to have proper initial parameters for MC and data (i.e. meanTimeMC=2ns and dataMC=0ns) but this would also contrast with the current choise of using the pulse template derived from the data even for MC.
I will remove this modification (c847539) from this PR but point to the HCAL-DPG conveners.
Let me know if you think differently.
There was a problem hiding this comment.
I will remove this modification (c847539) from this PR ... makes sense to me.
[some possibly relevant text follows]
Is p3 (the carpet plots) made for unconstrained chi2? (the formula on the page says so, but M2 we use in reco is constrained).
The slides from HCAL DPG p6 look like the same template is used for data and MC and the template matches data very well.
BTW, fine time binning or continuous curve overlay here would help to understand what is happening better. Due to very sharp rise of the expected pulse shape, anything significant that shows up to the left of the TS4 will very likely trigger 3-pulse fit which will call this early entry an OOT hit that we do not record as an output of the fit. The result may give smaller bias in time, but it will give a significant bias in energy. I'd sacrifice few ns bias in time for a decreased bias in energy. Calibrations can correct the biases, but it seems to me to be more important to integrate more true signal pulse as signal to not loose resolution in E and probably loose something in t.
This "timeMean" enters in two places:
- starting point for the fit
- constraint parameter for the fit
If MC disagrees so badly, then biases are "natural" and playing with fitting parameters without changing the expected shape is not very productive.
|
On 9/22/16 10:12 AM, mariadalfonso wrote:
OK, E=50 GeV. This doesn't really scan the full dynamic range I'm not sure of the meaning of this being run on SIM made with
|
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
|
|
On Thu, Sep 22, 2016 at 7:43 PM, Slava Krutelyov notifications@github.com
|
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
Pull request #15959 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
mariadalfonso
force-pushed
the
siPMfixes
branch
from
6368a32
to
79340b9
Compare
|
Pull request #15959 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
cmsbuild
added
comparison-pending
pending-signatures
reconstruction-pending
tests-pending
and removed
reconstruction-approved
tests-approved
labels
|
Pull request #15959 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
|
@davidlange6 I added the comment with the reference |
|
+1 for 37cc395
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
The tests are being triggered in jenkins. |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar |
|
Comparison job queued. |
|
Comparison is ready There are some workflows for which there are errors in the baseline: |
|
+1 |
Before the M2 siPM fits was done with the HPD pulse and parameters.
The save 20% of CPU time on single particle no PU
(http://dalfonso.web.cern.ch/dalfonso/DarkCurrentOnSiPM_PR15959.pdf)
This PR discuss also a possible move the initial time of the M2 fit to 2ns (Run2-phase1 ), but this is removed from the list of commit.
The save an extra 20% of CPU time on single particle no PU
Some reference slide
https://indico.cern.ch/event/570231/contributions/2306435/attachments/1341696/2021200/PR15959.pdf