CTPPS: bug fix in run ranges for FEDs channels mapping #18461

forthommel · 2017-04-25T12:53:35Z

This PR modifies the run ranges parser to account for scenarios where the first event number in a run is 0 instead of 1, thus preventing any channels mapping to be defined for this event.
It hence allows runs at the edge of these ranges to properly retrieve a channels mapping for both the strips and diamond detectors.
It also backports a bug fix raised in #18283 (comment) by @perrotta preventing the CRC check to be performed on the VFAT frame is the unpacker verbosity is set to 0.

runTheMatrix -l limited on bbc0068 yielded 9 9 8 6 4 1 1 1 tests passed, 1 0 0 0 0 0 0 0 failed (again, DAS_ERROR on 4.22)

…defined

cmsbuild · 2017-04-25T12:53:53Z

A new Pull Request was created by @forthommel (Laurent Forthomme) for CMSSW_8_0_X.

It involves the following packages:

EventFilter/CTPPSRawToDigi

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

cms-bot commands are listed here #13028

slava77 · 2017-04-25T12:59:54Z

EventFilter/CTPPSRawToDigi/python/ctppsRawToDigi_cff.py

      mappingFileNames = cms.vstring(),
      maskFileNames = cms.vstring()
    ),
    # after TS2 (2016)
    cms.PSet(
-      validityRange = cms.EventRange("281601:min - 999999999:max"),
+      validityRange = cms.EventRange("281600:min - 999999999:max"),


Now I'm missing how this code works: the previous range ends at 281600:max, while this range starts at 281600:min, which at least semantically suggests there is an overlap.
Please explain why there is no overlap and see if it's possible to remove it even in config semantics.

Hi @slava77
It seems that the bug lies further away than this, as the first event in /store/data/Run2016H/ZeroBias/RAW/v1/000/283/820/00000/462D2A5B-B19A-E611-B100-02163E01382E.root' (as discussed offline) is 283820:0:0, while 283820:min seems to correspond to 283820:0:1...

…the VFAT frames unpacking

…etrieval code

cmsbuild · 2017-04-25T13:43:09Z

Pull request #18461 was updated. @perrotta, @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @slava77, @ggovi, @mmusich, @davidlange6 can you please check and sign again.

forthommel · 2017-04-25T13:44:26Z

Commit bbc0068 fixes this unexpected behaviour when first event number observed is 0 instead of 1.

forthommel · 2017-04-25T13:50:44Z

type bugfix

slava77 · 2017-04-25T13:54:53Z

@cmsbuild please test

@forthommel please make a PR for 91X to follow the regular policy of changes

cmsbuild · 2017-04-25T13:55:11Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19382/console Started: 2017/04/25 15:55

cmsbuild · 2017-04-25T15:03:58Z

+1
Tested at: bbc0068
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18461/19382/summary.html

cmsbuild · 2017-04-25T15:04:01Z

Comparison job queued.

cmsbuild · 2017-04-25T15:36:28Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 16
DQMHistoTests: Total histograms compared: 1164335
DQMHistoTests: Total failures: 1176
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1163041
DQMHistoTests: Total skipped: 118
DQMHistoTests: Total Missing objects: 0
Checked 63 log files, 14 edm output root files, 16 DQM output files

slava77 · 2017-04-26T11:51:53Z

CondFormats/CTPPSReadoutObjects/plugins/TotemDAQMappingESSourceXML.cc

-    if (startEventID == EventID(1, 0, 1))
-      startEventID = EventID(1, 0, 0);
+    if (startEventID.event() == 1)
+      startEventID = EventID(startEventID.run(), startEventID.luminosityBlock(), 0);


I suspect that "1" in "1:min" was not special and the comment should be
// event id "N:min" has a special meaning and is translated to a truly minimal event id (N:0:0)
We have clearly discovered in MC that the first call is made not inside an event but inside a beginRun transition to make this comment for MC. This PR is apparently the first time we tried this code on the transition run.

If you look in the job failing in 283820 the file first entry is the run transition

283820 0 0 0 (Run)

perrotta · 2017-04-26T15:02:42Z

+1
Tested in CMSSW_8_0_27 with the same input file where the crash was experienced:
/store/data/Run2016H/ZeroBias/RAW/v1/000/283/820/00000/462D2A5B-B19A-E611-B100-02163E01382E.root
this PR actually fixes it.

arunhep · 2017-04-26T15:04:30Z

+1

davidlange6 · 2017-05-15T08:11:49Z

hi @forthommel looks like we failed to notice that this PR still does not handle begin/end conditions correctly. The code presumably was written for run ranges rather than the more complicated EventRanges and the evolution is still not complete.

Please change to use the contains function defined in the EventRange header (its not clear to me why there is no contains method of the class it self but there is not..)

See:

https://hypernews.cern.ch/HyperNews/CMS/get/prep-ops/3940/1/1/1/1/1.html

forthommel · 2017-05-15T09:16:52Z

Hi @davidlange6
As shown above, another bug fix has been submitted for reviewing in 8_0_X (#18732). It implements your suggestion of replacing our home-made run ranges check with the centrally-provided edm::contains method.

Let me know whenever I can forward-port this fix onto the master branch.

davidlange6 · 2017-05-15T09:20:42Z

if you've confirmed that this can now process 280385, then indeed, we need the usual master branch (and probably 91x) pull requests

…

On May 15, 2017, at 11:16 AM, Laurent Forthomme ***@***.***> wrote: Hi @davidlange6 As shown above, another bug fix has been submitted for reviewing in 8_0_X (#18732). It implements your suggestion of replacing our home-made run ranges check with the centrally-provided edm::contains method. Let me know whenever I can forward-port this fix onto the master branch. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

forthommel · 2017-05-15T09:36:18Z

Indeed, I am now successfully processing this run with the PSet configuration attached to the HN thread you quoted (currently at event 210).
I will then launch the forward-port(s).

Modified the run ranges at which the diamond and strips mappings are …

94fd638

…defined

cmsbuild added this to the Next CMSSW_8_0_X milestone Apr 25, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 25, 2017

slava77 reviewed Apr 25, 2017

View reviewed changes

forthommel added 2 commits April 25, 2017 15:01

Backport of the fix to the bug preventing to check for CRC errors on …

3cceaaa

…the VFAT frames unpacking

First event in run is set to 0 instead of 1 in the channels mapping r…

bbc0068

…etrieval code

cmsbuild added alca-pending db-pending labels Apr 25, 2017

forthommel changed the title ~~CTPPS: modified run ranges for FEDs channels mapping~~ CTPPS: bug fix in run ranges for FEDs channels mapping Apr 25, 2017

cmsbuild added bug-fix tests-started and removed tests-pending labels Apr 25, 2017

forthommel mentioned this pull request Apr 25, 2017

CTPPS: bug fix in run ranges for FEDs channels mapping #18464

Merged

cmsbuild added tests-approved and removed tests-started labels Apr 25, 2017

jan-kaspar mentioned this pull request Apr 25, 2017

CTPPS: strips update for 2017 data-taking #18283

Merged

cmsbuild removed the comparison-pending label Apr 25, 2017

cmsbuild added the comparison-available label Apr 25, 2017

slava77 reviewed Apr 26, 2017

View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Apr 26, 2017

cmsbuild added alca-approved and removed alca-pending labels Apr 26, 2017

perrotta mentioned this pull request Apr 26, 2017

Crash in ZdcHitReconstructor #18463

Closed

davidlange6 merged commit 897c137 into cms-sw:CMSSW_8_0_X Apr 27, 2017

forthommel mentioned this pull request May 15, 2017

CTPPS: bug fix #2 in run ranges for FEDs channels mapping #18732

Merged

forthommel deleted the ctpps-runranges_channels_patch-8_0_X branch July 17, 2017 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CTPPS: bug fix in run ranges for FEDs channels mapping #18461

CTPPS: bug fix in run ranges for FEDs channels mapping #18461

forthommel commented Apr 25, 2017 •

edited

cmsbuild commented Apr 25, 2017

slava77 Apr 25, 2017

forthommel Apr 25, 2017

cmsbuild commented Apr 25, 2017

forthommel commented Apr 25, 2017

forthommel commented Apr 25, 2017

slava77 commented Apr 25, 2017

cmsbuild commented Apr 25, 2017 •

edited

cmsbuild commented Apr 25, 2017

cmsbuild commented Apr 25, 2017

cmsbuild commented Apr 25, 2017

slava77 Apr 26, 2017

perrotta commented Apr 26, 2017

arunhep commented Apr 26, 2017

davidlange6 commented May 15, 2017

forthommel commented May 15, 2017

davidlange6 commented May 15, 2017 via email

forthommel commented May 15, 2017

CTPPS: bug fix in run ranges for FEDs channels mapping #18461

CTPPS: bug fix in run ranges for FEDs channels mapping #18461

Conversation

forthommel commented Apr 25, 2017 • edited

cmsbuild commented Apr 25, 2017

slava77 Apr 25, 2017

Choose a reason for hiding this comment

forthommel Apr 25, 2017

Choose a reason for hiding this comment

cmsbuild commented Apr 25, 2017

forthommel commented Apr 25, 2017

forthommel commented Apr 25, 2017

slava77 commented Apr 25, 2017

cmsbuild commented Apr 25, 2017 • edited

cmsbuild commented Apr 25, 2017

cmsbuild commented Apr 25, 2017

cmsbuild commented Apr 25, 2017

slava77 Apr 26, 2017

Choose a reason for hiding this comment

perrotta commented Apr 26, 2017

arunhep commented Apr 26, 2017

davidlange6 commented May 15, 2017

forthommel commented May 15, 2017

davidlange6 commented May 15, 2017 via email

forthommel commented May 15, 2017

forthommel commented Apr 25, 2017 •

edited

cmsbuild commented Apr 25, 2017 •

edited