[backport] Set equivalence of first/last bx labels used by different subsystems in ECAL DQM #35674

alejands · 2021-10-14T15:16:37Z

PR description:

A bug was discovered by ECAL DAQ in which the labeling conventions for the first/last bunch crossing are different depending on the subsystem: http://cmsonline.cern.ch/cms-elog/1123223

TCDS always labels this bx as 3564, while other subsystems label it as 0. This was leading to a DQM integrity error plot being mistakenly filled.

We have set these values so the bx labels are treated as the same in this scope and avoid incorrectly filling DQM integrity error plots.

PR validation:

The ECAL DQM code was run on an offending dataset and the fake integrity errors disappeared. We also confirmed the results with ECAL DAQ.

if this PR is a backport please specify the original PR and why you need to backport that PR:

This is a backport of #35661 to include it in Online DQM at P5.

cmsbuild · 2021-10-14T15:17:00Z

A new Pull Request was created by @alejands (Alejandro Sanchez) for CMSSW_12_0_X.

It involves the following packages:

DQM/EcalMonitorTasks (dqm)

@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@rchatter, @simonepigazzini, @thomreis, @argiro this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Backported from Set equivalence of first/last bx labels used by different subsystems in ECAL DQM #35661

jfernan2 · 2021-10-14T15:36:59Z

test parameters

addpkg=DQM/Integration

jfernan2 · 2021-10-14T15:38:33Z

please test

cmsbuild · 2021-10-14T18:49:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c05eb2/19636/summary.html
COMMIT: 2def7d9
CMSSW: CMSSW_12_0_X_2021-10-14-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35674/19636/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2998564
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2998542
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

perrotta · 2021-10-15T06:24:35Z

backport of #35661

jfernan2 · 2021-10-18T06:35:30Z

+1
Tested in Online DQM at P5

cmsbuild · 2021-10-18T06:35:54Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_1_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-10-18T13:46:36Z

DQM/EcalMonitorTasks/src/RawDataTask.cc

+    // TCDS calls it always 3564, but some subsystems call it 0.
+    // From testing: bx_ is labeled 0, dccBX and FEBxs[iFE] labeled 3564
+    // Setting bx_ to 0 to match the other two
+    if (bx_ == 3564)


Instead of that magic number, it would be preferable to rely to some centralized constant, for example LumiConstants::numBX taken from DataFormats/Luminosity/interface/LumiConstants.h

I would personally do the same also in the master version of this PR

I agree that this is much safer and better practice. I've made the change, but I will run a test as a sanity check before pushing to this PR.

Unfortunately, it appears the master version of this PR is already merged, but I can quickly submit a new PR with the change. Alternatively, I can hold onto the change in master for now and submit it in an upcoming PR.

Unfortunately, it appears the master version of this PR is already merged, but I can quickly submit a new PR with the change. Alternatively, I can hold onto the change in master for now and submit it in an upcoming PR.

Thank yuo @alejands
Please just add a quick fix with a new PR in the master

Hi @perrotta

I've pushed the change you suggested regarding using a centrally defined constant rather than a hard-coded number

…Luminosity/interface/LumiConstants.h

cmsbuild · 2021-10-18T21:39:15Z

Pull request #35674 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please check and sign again.

perrotta · 2021-10-19T05:26:27Z

please test

perrotta · 2021-10-19T07:37:54Z

@alejands, I see no update PR for the master yet.
Then I prepared a quick one myself, see #35725

cmsbuild · 2021-10-19T10:11:55Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c05eb2/19728/summary.html
COMMIT: a769621
CMSSW: CMSSW_12_0_X_2021-10-18-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35674/19728/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2998564
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2998530
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 38 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

alejands · 2021-10-19T12:28:31Z

@perrotta Thanks for submitting the update for master. There was an issue brought up last night #35723 so I was running a test overnight out of an abundance of caution before submitting another PR. It appears it's all clear and good to go.

jfernan2 · 2021-10-19T16:18:05Z

+1
Resigning after new test in P5

cmsbuild · 2021-10-19T16:18:28Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_1_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-10-20T08:27:57Z

+1

Both PRs from which this one is backported, Set equivalence of first/last bx labels used by different subsystems in ECAL DQM #35661 and Change bx constant to centrally defined value in RawDataTask.cc #35725, are now succesfully merged in 12_1

set equivalence of first/last bx labels used by different subsystems

2def7d9

cmsbuild added this to the CMSSW_12_0_X milestone Oct 14, 2021

alejands marked this pull request as ready for review October 14, 2021 15:16

cmsbuild added dqm-pending orp-pending pending-signatures tests-pending labels Oct 14, 2021

alejands mentioned this pull request Oct 14, 2021

Set equivalence of first/last bx labels used by different subsystems in ECAL DQM #35661

Merged

cmsbuild added tests-started and removed tests-pending labels Oct 14, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 14, 2021

cmsbuild added the backport label Oct 15, 2021

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Oct 18, 2021

cmsbuild added backport-ok and removed backport labels Oct 18, 2021

perrotta reviewed Oct 18, 2021

View reviewed changes

change bx constant to centrally defined value defined in DataFormats/…

a769621

…Luminosity/interface/LumiConstants.h

cmsbuild removed dqm-approved fully-signed labels Oct 18, 2021

cmsbuild added dqm-pending pending-signatures tests-pending and removed tests-approved labels Oct 18, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 19, 2021

perrotta mentioned this pull request Oct 19, 2021

Change bx constant to centrally defined value in RawDataTask.cc #35725

Merged

cmsbuild added tests-approved and removed tests-started labels Oct 19, 2021

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Oct 19, 2021

cmsbuild added orp-approved and removed orp-pending labels Oct 20, 2021

cmsbuild merged commit 0a2185c into cms-sw:CMSSW_12_0_X Oct 20, 2021

alejands deleted the bxFirstLastLabels_120X branch November 1, 2021 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[backport] Set equivalence of first/last bx labels used by different subsystems in ECAL DQM #35674

[backport] Set equivalence of first/last bx labels used by different subsystems in ECAL DQM #35674

alejands commented Oct 14, 2021

cmsbuild commented Oct 14, 2021 •

edited

Loading

jfernan2 commented Oct 14, 2021

jfernan2 commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

perrotta commented Oct 15, 2021

jfernan2 commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

perrotta Oct 18, 2021

alejands Oct 18, 2021

perrotta Oct 18, 2021

alejands Oct 18, 2021

cmsbuild commented Oct 18, 2021

perrotta commented Oct 19, 2021

perrotta commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

alejands commented Oct 19, 2021 •

edited

Loading

jfernan2 commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

perrotta commented Oct 20, 2021

[backport] Set equivalence of first/last bx labels used by different subsystems in ECAL DQM #35674

[backport] Set equivalence of first/last bx labels used by different subsystems in ECAL DQM #35674

Conversation

alejands commented Oct 14, 2021

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Oct 14, 2021 • edited Loading

jfernan2 commented Oct 14, 2021

jfernan2 commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

Comparison Summary

perrotta commented Oct 15, 2021

jfernan2 commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

perrotta Oct 18, 2021

Choose a reason for hiding this comment

alejands Oct 18, 2021

Choose a reason for hiding this comment

perrotta Oct 18, 2021

Choose a reason for hiding this comment

alejands Oct 18, 2021

Choose a reason for hiding this comment

cmsbuild commented Oct 18, 2021

perrotta commented Oct 19, 2021

perrotta commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

Comparison Summary

alejands commented Oct 19, 2021 • edited Loading

jfernan2 commented Oct 19, 2021

cmsbuild commented Oct 19, 2021

perrotta commented Oct 20, 2021

cmsbuild commented Oct 14, 2021 •

edited

Loading

alejands commented Oct 19, 2021 •

edited

Loading