SiPM pulse shape fixes #16825
SiPM pulse shape fixes #16825
Conversation
|
A new Pull Request was created by @kpedro88 (Kevin Pedro) for CMSSW_9_0_X. It involves the following packages: CalibCalorimetry/HcalAlgos @ghellwig, @civanch, @arunhep, @cerminar, @cmsbuild, @franzoni, @mdhildreth, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready The workflows 1003.0, 1001.0, 1000.0, 140.53, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons |
|
The changes in the comparisons for 2017 (and 2023) look reasonable to me. Other MC workflows show apparently-unrelated minor changes due to random number shifting. (The SiPM simulation is rather complex, with random number calls dependent on other random number calls that depend on parameters, so any parameter change can shift the whole chain.) |
|
in 10024 I see Maria's plot moved in the opposite direction Am I just comparing different phase space or other kind of apples to oranges? |
|
@slava77 the major differences are sample type (500 GeV pion vs ttbar) and size (1000 events vs 10 events). I made the set of M2 plots using baseline and this PR from 10024 step3.root. The performance overall agrees with what I expect: reduced chi2 at high energy (low energy has a larger spread in the baseline, which is not surprising - this PR mostly serves to improve self-consistency) and less spread in the time distribution. |
|
+1 |
|
I'm not sure about the current plans for 81X vs 90X for 2017 tuning. If we no longer need to have the best local reco in 81X, the backport is unnecessary. But the POGs/DPGs still seem to be focusing on 81X as the standard for 2017... |
|
@mmusich @davidlange6 I would like to get this into pre2 if possible |
|
+1 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar |
Some arbitrary cutoffs on the ranges of the Y11 and SiPM pulse shapes used in the digi simulation had been present in the code for several years. These cutoffs caused unphysical dropoffs in the output pulses, so they are now removed. Correspondingly, the RECO shape used for M2 is updated to be a full 250ns convolution.
Extensive investigation of these issues can be found in these slides: sipm_pulse_study_4.pdf. Plots of M2 performance are included. Additionally, here is a 50 GeV pion response/resolution plot ("ref" corresponds to 810pre16 + #16569, "new" is ref + this PR):
NB: Changing
Y11RANGEglobally will also change HO simulation output for Run1 and Run2. Theoretically this is more accurate, so I think the change should be allowed. If it is not acceptable, as a hack I can resetY11RANGEon a per-channel basis, but I would prefer not to do this.Planned items for followup PRs:
HcalSiPMShapeHcalSiPMShapeintoCalibCalorimetry/HcalAlgosHcalSiPMShapeto calculate numerical convolution for RECO shape on demand (replacing array of values)vector<CaloSamples>as event product for easier debuggingThis PR will be backported to 81X.