-
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 DQM remove OneLumiEDAnalyzer #29544
Conversation
The code-checks are being triggered in jenkins. |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29544/14828
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-29544/14830
|
A new Pull Request was created by @lwang046 for master. It involves the following packages: DQM/HcalCommon @andrius-k, @kmaeshima, @schneiml, @cmsbuild, @jfernan2, @fioriNTU can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins. |
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@lwang046 I understand that DigiTask contains the old/reduced DigiPhase1Task. |
On a totally different subject: the changes affecting to Online DQM are not and cannot be tested here in github. And we (DQM) cannot test them since P5 is down at present. So, I understand you have made some independent validation that the plots including per LS/events are remaining unchanged. |
Related to my last comment: the reportSummaryMap which contains info vs LS changes already at Offline |
Another strange fact is double number of processed events in non-Phase2 workflows, eg.: |
Hi @jfernan2, the DigiRunHarvesting folder comes from here: https://github.com/cms-sw/cmssw/blob/master/DQM/HcalTasks/plugins/HcalOfflineHarvesting.cc#L13-L24. Previously with the DigiPhase1Task the |
Forgiving me for a stupid question, if the same ME was booked twice: cmssw/DQM/HcalTasks/plugins/DigiTask.cc Lines 789 to 790 in 7bf830b
cmssw/DQM/HcalTasks/plugins/DigiPhase1Task.cc Lines 297 to 298 in 7bf830b
NB: I did a local test with the master branch by commenting out one of these 2 same ME filling cases and it confirmed that previously it was filled twice. I guess this was overlooked before when the DigiPhase1Task was used as an assistant module. But now the event number is correct. |
@jfernan2 My apologies I failed to understand your comments here, may I ask which checks you were looking for? If the question was why now we have an extra DIGI on Y-axis, the reason is very similar to the DigiRunHarvesting folder issue, now that we replaced DigiPhase1Task with DigiTask, the DIGI condition ( cmssw/DQM/HcalTasks/plugins/HcalOfflineHarvesting.cc Lines 77 to 79 in b13393b
|
Thanks a lot @lwang046 Hence ,in Online DQM you should probably test the changes running standalone the two clients |
+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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
It seems that this PR might have caused 3 workflows to segfault in HARVESTING step, see #29605. |
Hi @lwang046, to have this clarified:
If you book the same name twice, you get the same object in both places. If you then call fill from two modules, you see all the data in one histogram. That is how the "Number of Events" histograms showed double number of events. This behavior changed recently (in the new DQMStore work, merged in 11_1), and these plots were among the few changes noticed there. |
Resolves #25090 |
A fix to Hcal related legacy modules raised in issues #25090.
Passed local
runTheMatrix.py -l limited -i all --ibeos
test without fails.