-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Update new Phase2C11xx eras by adding the etlV4 modifier #33139
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33139/21500
|
A new Pull Request was created by @fabiocos (Fabio Cossutti) for master. It involves the following packages: Configuration/Eras @jordan-martins, @chayanit, @wajidalikhan, @srimanob, @kpedro88, @cmsbuild, @silviodonato, @franzoni, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-96315f/13425/summary.html Comparison SummarySummary:
|
Thanks @fabiocos |
@srimanob I'll check and remove them. @silviodonato @qliphy not sure why there are so many changes in the tests, of course D76 will be affected in ETL and whatever depends on it, but I do not understand why Hcal digitization should be affected for instance |
@silviodonato @qliphy ok, the random number sequence for digitization is now common to most subdetectors, and this may explain what is observed (muons are separate, and indeed muon digitization does not see significant changes). |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33139/21513
|
@smuzaffar thanks for understanding the issue, I do not recall it in the past, and the number of failures here does not seem particularly big compared to other situations. I'll wait for the end of the new comparison |
@fabiocos , histogram are available now |
Hi @fabiocos |
@srimanob from a quick inspection you may see that the BTL plots are essentially unchanged (but for statistical fluctuations), as expected. As far as ETL is concerned, the charge deposit is significantly different: as expected from the activation of https://github.com/cms-sw/cmssw/blob/master/SimFastTiming/FastTimingCommon/python/mtdDigitizer_cfi.py#L88 Further more, the hits associated to tracks (negative values for ETL, see https://github.com/cms-sw/cmssw/blob/master/Validation/MtdValidation/plugins/MtdGlobalRecoValidation.cc#L209 ) are now higher than before: @gsorrentino18 please confirm that this is the expected behaviour, based on your comparison with the old D73 plots |
+Upgrade |
@fabiocos yes, I confirm this is the same behavior I observed when I made comparisons with D73 scenario |
type bug |
Urgent Should be included in pre5. FYI @qliphy @silviodonato |
+1 |
+operations |
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
In the recent addition of new upgrade geometry scenarios and Eras based on MTD version I13 , following the example of D73, the addition of https://github.com/cms-sw/cmssw/blob/master/Configuration/Eras/python/Modifier_phase2_etlV4_cff.py has been overlooked. This causes a significant drop in efficiency of ETL reconstructed hits for instance in scenario D76, due incorrect digitization, as mentioned in #33130 .
This PR fixes the issue by defining new Eras with the proper addition of the requested modifier, and updating the corresponding test workflows accordingly.
PR validation:
Test wf. 34634.0 now shows back a reasonable hit efficiency in ETL on 10 TTbar events.