-
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
Use ECAL ratio timing algorithm for Run 1 and Run 2, and CC timing algorithm for Run 3 and beyond - 132X #42946
Use ECAL ratio timing algorithm for Run 1 and Run 2, and CC timing algorithm for Run 3 and beyond - 132X #42946
Conversation
…3 and use ratio timing for Run1 and Run2.
A new Pull Request was created by @thomreis (Thomas Reis) for CMSSW_13_2_X. It involves the following packages:
@davidlange6, @fabiocos, @antoniovilela, @mandrenguyen, @cmsbuild, @rappoccio, @jfernan2 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
type ecal |
backport of #42928 |
assign alca
|
New categories assigned: alca @saumyaphor4252,@perrotta,@consuegs you have been requested to review this Pull request/Issue and eventually sign? Thanks |
please test |
-1 Failed Tests: UnitTests RelVals RelVals-INPUT AddOn Unit TestsI found 3 errors in the following unit tests: ---> test TestDQMOnlineClient-ecal_dqm_sourceclient had ERRORS ---> test TestDQMOnlineClient-visualization had ERRORS ---> test TestDQMOnlineClient-visualization_secondInstance had ERRORS RelVals
Expand to see more relval errors ...RelVals-INPUT
AddOn Tests
Expand to see more addon errors ... |
This PR needs GT updates like in #42958 in order to pass the tests. |
please test |
Hi @perrotta our validators are looking at the this at the moment. |
Hi, we are starting to see good reports for 13_3_0_pre4 EGM so most probably we could include this, unless I'm missing something. Then, we (PdmV) would cut a 13_2_X release with this in to have a small validation from EGM people also for 13_2_X (analogously to what is being done for 13_1_X). What do you think? We can discuss it at the next ORP, of course. |
Thank you @AdrianoDee. The possible issue with 13_2_X is that it is a production release, already used for HI data taking, and going to be put in production for the HI MC. I would wait for a full green light from the validators in 13_3_0_pre4 (as far as I understand, "we are starting to see good reports" does not mean the validation is already successfully ended). I think that at that point we can proceed with including this in 13_2_X, and produce the small validation sample for EGM that you are suggesting, which is a welcome last check for this. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fe4774/36074/summary.html Comparison SummarySummary:
|
@perrotta fair enough. Yes, not the full release, for the moment I was referring to the EGM reports (that were the failing ones due to timing algo changes). |
please test |
All the latest reports from EGM including this PR are green: This was the case for 2023 also in 13_3_0_pre4 (the first pre where the original PR was included). In general for 13_3_0_pre4 (opened 1 month ago) there is no failure report for 2023. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fe4774/36544/summary.html Comparison SummarySummary:
|
+alca
|
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_13_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_14_0_X is complete. This pull request will be automatically merged. |
PR description:
This PR sets the default ECAL timing algorithm to the ratio method. This is the default timing algorithm for Run 1 and Run 2.
For Run 3 and Phase 2 a new modifier
run3_ecal
is used to change the timing algorithm to CC timing and use different records with the label 'CC' for the timing calibrations and timing offset constants.Note that this behaviour is different than what was discussed in the meeting on the 27th Sept. when the plan was made to make the CC timing the default and use eras to modify Run 1 and Run 2 configurations to use the ratio method. Since Run 1 configurations could not be modified using eras we decided to reverse the behaviour as in the description above.
Backport of #42928
PR validation:
The PR passes Run 1 and Run2 WFs in the limited matrix tests but currently fails all Run 3 and Phase 2 WFs because the consumed records with the 'CC' label are not yet in the GTs.