-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HCAL: implementation of TDC simulation functionality for Phase1 HB/HE #28330
Conversation
The code-checks are being triggered in jenkins. |
Long, you might want to edit the title to smth. more self-explanatory, e.g.: "HCAL: implementation of TDC functionality for Phase1 HB/HE" |
ups.. TDC -> TDC simulaton... |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28330/12586
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28330/12587
|
A new Pull Request was created by @lwang046 for master. It involves the following packages: DataFormats/HcalDigi @perrotta, @cmsbuild, @civanch, @mdhildreth, @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. |
DQM comparisons in the jenkins automatic tests show differences, which are quite likely explained by the increased TDC threshold set. E.g. for wf 10824.0 HEM entries lower from 14952 to 772, with a completely different time distribution: @lwang046 can you confirm that plots as above (as well al all the other different ones visible in the test outputs) are in fact expected? And, just for my own education, could you please explain what do they describe, and what exactly determines those differences? |
@perrotta |
In fact, tests do not show any difference in reco outputs. I was only puzzled by the ones showing up in DQM. Thank you Salavat for explaining their origin |
...Clarification: Mixing continues to use TDC bits, but now (with this PR) at the end of Digitization (after Mixing) comes TDC simulation, which fills these bits with regular TDC info before packing (Digi2Raw)... |
+1
|
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 |
Since the merging yesterday of this PR, tests in other pull requests show differences in the DQM outputs for HCal TDC's. See for example the test results of #28363, where workflows with and without pileup have this issue. As you already pointed out earlier in this thread, this does not affect reco outputs. However, it seems to point to some non-reproducibility of the TDC results provided by this PR, which should be better understood. Could you please have a look and provide an explanation (and possibly also a fix) for such a non-reproducibility? |
@perrotta it seems we arrive at the same conclusion |
@perrotta |
@abdoulline changes in LETDCTime histograms for DigiTask in a geometry PR for DD4hep... https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_0_X_2019-11-07-1100+cfae32/34131/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017+NanoFull_2017/Hcal_DigiTask.html |
So, this is 10024.0 2017... |
Just for laying down the observations: I must admit I don't understand the origin of issue, it might be related to DQM deliberate back-incompatibility (unlike DQMOffline) and it's primary data "orientation" (we're not using it for MC RelVals). A fairly quick workaround could be that we can switch off TDC simulation for all eras, |
PR description:
1,Update of the HB/HE TDC simulation
The current version of HcalTDC.cc was developed but obsolete and never used in simulation production. The algorithm in it is rewritten to provide TDC timing according to the specifications of QIE11 running at p5, and the timing method is now used when simulating hcal electronics to get the TDC time
2, Modification of MC Digis packing (PackerHelp.h and HCalDigiToRawuHTR.cc)
Convert the data format according to uHTR specifications for HB
3, Making TDC current cut a configurable (HcalElectronicsSim.cc and some other files)
Passed a new variable to the constructor to allow for tuning of TDC current threshold from python configuration files.
PR validation:
(1) pion-gun results are identical w/wo the TDC branch in 11_0_0_pre11: results here
(2) TDC has been tested privately and yields expected results; eg. page 5 here
(3) runTheMatrix.py -s test is OK.