GEM - fed data size bugfix [EventFilter/GEMRawToDigi] #31776

jshlee · 2020-10-13T19:26:43Z

PR description:

error in how FED trailer fragmentLength was stored

PR validation:

only changes 1 number in FED data that isn't used

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

no backport needed

cmsbuild · 2020-10-13T19:27:07Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-10-13T19:32:20Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31776/19026

This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31776/19026/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31776/19026/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2020-10-13T19:34:18Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-10-13T19:40:13Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31776/19027

This PR adds an extra 20KB to repository

cmsbuild · 2020-10-13T19:40:39Z

A new Pull Request was created by @jshlee (Jason Lee) for master.

It involves the following packages:

EventFilter/GEMRawToDigi

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @watson-ij this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

perrotta · 2020-10-14T05:43:21Z

EventFilter/GEMRawToDigi/plugins/GEMDigiToRawModule.cc

@@ -266,9 +268,12 @@ void GEMDigiToRawModule::produce(edm::StreamID iID, edm::Event& iEvent, edm::Eve
    fedRawData.resize(dataSize);

    uint64_t* w = reinterpret_cast<uint64_t*>(fedRawData.data());
-    for (const auto& word : words)
+    for (const auto& word : words) {
+#ifdef EDM_ML_DEBUG


It is curious that you use both LogDebug and #ifdef EDM_ML_DEBUG in the same class: was any of them a remnant of your validation of the fix, or are they supposed to remain both?
In any case, a

#include "FWCore/MessageLogger/interface/MessageLogger.h"

statement is missing from both plugins

@perrotta Only LogDebug was used originally. I added a few more LogDebug and used EDM_ML_DEBUG for the output to be easier to see. I can change them all to either one if that is preferred.

I think that having one single method to report debug logs can be less confusing, and the one stated in the MessageLogger are typically recommendedin CMSSW instead of cout's (which aren't thread safe, in case you want to use them in a multithread environment). I won't be strict on it, though, and I just wanted to point it out to you.
In any case, if you stick on LogDebug, please add the missing header includes

cmsbuild · 2020-10-14T06:17:41Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-10-14T06:24:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31776/19038

This PR adds an extra 20KB to repository

perrotta · 2020-10-14T06:24:29Z

please test

cmsbuild · 2020-10-14T06:24:35Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-10-13-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9933/console Started: 2020/10/14 08:25

cmsbuild · 2020-10-14T06:24:36Z

Pull request #31776 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

cmsbuild · 2020-10-14T07:35:35Z

+1
Tested at: d2467cd
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-96d424/9933/summary.html
CMSSW: CMSSW_11_2_X_2020-10-13-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-10-14T07:35:38Z

Comparison job queued.

cmsbuild · 2020-10-14T08:51:26Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-96d424/9933/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2543752
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2543723
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

perrotta · 2020-10-14T08:55:45Z

+1

Simple and logical fix
Jenkins tests pass

cmsbuild · 2020-10-14T08:56:07Z

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)

silviodonato · 2020-10-14T09:39:58Z

+1
if I understand correctly, this should affect both Run-3 and Phase-2. Please consider to backport this bugfix to 11_1_X both for the Phase-2 HLT TDR (@trtomei @fwyzard ) and for the MWGR (@amassiro @boudoul)

fwyzard · 2020-10-14T10:05:22Z

hi Silvio, I'm not against a backport, but to make use of these changes we would need to generate new MC RAW data...

silviodonato · 2020-10-14T10:10:10Z

hi Silvio, I'm not against a backport, but to make use of these changes we would need to generate new MC RAW data...

Right, the fix is in DigiToRaw, not RawToDigi !

jshlee · 2020-10-15T04:33:35Z

I don't see the need for this to be backported as this variable (FED trailer fragmentLength) isn't used in RawToDigi process.

bug in counting fed data size

9873e80

cmsbuild added this to the CMSSW_11_2_X milestone Oct 13, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 13, 2020

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 13, 2020

code-format

d7eefac

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 13, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 13, 2020

perrotta reviewed Oct 14, 2020

View reviewed changes

added MessageLogger.h

d2467cd

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 14, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 14, 2020

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

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

cmsbuild added comparison-available and removed comparison-pending labels Oct 14, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Oct 14, 2020

cmsbuild added orp-approved and removed orp-pending labels Oct 14, 2020

cmsbuild merged commit d3fe185 into cms-sw:master Oct 14, 2020

silviodonato changed the title ~~GEM - fed data size~~ GEM - fed data size bugfix [EventFilter/GEMRawToDigi] Oct 14, 2020

This was referenced Oct 15, 2020

Removed use of boost::any in edmNew::DetSetVector #31792

Merged

Add ParticleNetAK4 to JME custom NanoAOD #31714

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEM - fed data size bugfix [EventFilter/GEMRawToDigi] #31776

GEM - fed data size bugfix [EventFilter/GEMRawToDigi] #31776

jshlee commented Oct 13, 2020

cmsbuild commented Oct 13, 2020

cmsbuild commented Oct 13, 2020

cmsbuild commented Oct 13, 2020

cmsbuild commented Oct 13, 2020

cmsbuild commented Oct 13, 2020

perrotta Oct 14, 2020 •

edited

Loading

jshlee Oct 14, 2020

perrotta Oct 14, 2020

jshlee Oct 14, 2020

cmsbuild commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

perrotta commented Oct 14, 2020

cmsbuild commented Oct 14, 2020 •

edited

Loading

cmsbuild commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

perrotta commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

silviodonato commented Oct 14, 2020

fwyzard commented Oct 14, 2020

silviodonato commented Oct 14, 2020

jshlee commented Oct 15, 2020

GEM - fed data size bugfix [EventFilter/GEMRawToDigi] #31776

GEM - fed data size bugfix [EventFilter/GEMRawToDigi] #31776

Conversation

jshlee commented Oct 13, 2020

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 13, 2020

cmsbuild commented Oct 13, 2020

cmsbuild commented Oct 13, 2020

cmsbuild commented Oct 13, 2020

cmsbuild commented Oct 13, 2020

perrotta Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

jshlee Oct 14, 2020

Choose a reason for hiding this comment

perrotta Oct 14, 2020

Choose a reason for hiding this comment

jshlee Oct 14, 2020

Choose a reason for hiding this comment

cmsbuild commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

perrotta commented Oct 14, 2020

cmsbuild commented Oct 14, 2020 • edited Loading

cmsbuild commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

perrotta commented Oct 14, 2020

cmsbuild commented Oct 14, 2020

silviodonato commented Oct 14, 2020

fwyzard commented Oct 14, 2020

silviodonato commented Oct 14, 2020

jshlee commented Oct 15, 2020

perrotta Oct 14, 2020 •

edited

Loading

cmsbuild commented Oct 14, 2020 •

edited

Loading