Skip to content
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

[10_1_X] SiStrip (un)packer: fixes and support for nonstandard ZS(lite) modes #23620

Merged

Conversation

pieterdavid
Copy link
Contributor

@pieterdavid pieterdavid commented Jun 19, 2018

backport of #23417
(as discussed in last Friday's reco meeting (see this contribution))

No changes in the tests are expected, except (as in the 10_2 PR) for warnings:

  • in 140.53 (repacked 2011 HI data) about an invalid packet code (0 for non-lite zero-suppressed data), which is due to a pre-2015 packer bug that is worked around in the unpacker
  • about missing data from FED 87 for 1000.0 and 1001.0 (2011 minimum bias, run 165121), and from FEDs 87, 135 and 137 for the 4.22 test (2011 cosmics, run 160960). These were silenced before, and are all understood and not related to the changes done here.

CC: @alesaggio @OlivierBondu @vidalm @mmusich @echabert @erikbutz

pieterdavid and others added 20 commits June 19, 2018 09:05
- rename the PACKET_CODE definitions for ZS lite to ZS
- change the packetCode implementation: default (0) for ZS lite modes,
  taken from the third byte for ZS
Reason: for ZS lite there are no packet codes, the packing scheme
is given by the readout mode; for non-lite ZS it is in the channel data
- add "PacketCode" option to SiStripDigiToRawModule
- add packetCodeFromString conversion in SiStripFEDBufferComponents
- pass the packet code on to FEDBufferPayloadCreator::fillChannelBuffer
workaround for a bug in the pre-2015 packer (eg. 140.53_RunHI2011)
In debug mode all messages are still printed, otherwise a summary
(warning type and count) is printed after unpacking the event.
@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 19, 2018

A new Pull Request was created by @pieterdavid (Pieter David) for CMSSW_10_1_X.

It involves the following packages:

Configuration/StandardSequences
EventFilter/SiStripRawToDigi
RecoLocalTracker/SiPixelClusterizer
SimTracker/SiPixelDigitizer

@perrotta, @civanch, @mdhildreth, @cmsbuild, @franzoni, @slava77, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @dkotlins, @gpetruc, @ebrondol, @threus, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2523210
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2523033
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 28 files compared)
  • Checked 119 log files, 9 edm output root files, 29 DQM output files

@perrotta
Copy link
Contributor

+1

  • This is identical commit-wise and code wise to SiStrip (un)packer: fixes and support for nonstandard ZS(lite) modes #23417, now merged in the master
  • SiStrip unpacker adapted for non-standard ZS modes as described: user tests show that the unpacked data reproduce the input ones
  • No effect is visible in standard workflows, where those non-standard ZS modes are not at work
  • Warning messages have also been reorganized and improved: this generates an extra warning (once per job) in the 2011 pp workflows. It was investigated, but it remains outside the scope of this PR

@civanch
Copy link
Contributor

civanch commented Jun 19, 2018

+1

@boudoul
Copy link
Contributor

boudoul commented Jun 26, 2018

Dear @fabiocos - Would it be possible to get this integrated in the next 10_1_X in order to make it available for operation ? Thanks

@slava77
Copy link
Contributor

slava77 commented Jul 9, 2018

@fabiocos
please clarify the plans for this PR, if it will be added to 10_1_X soon.
Thank you.

@mmusich
Copy link
Contributor

mmusich commented Jul 12, 2018

@fabiocos may I ask if there is at this point any plan to include this PR in the 10_1_X cycle or shall we just have in post-TS2 data in 10_2_X? This is somehow a nuisance for continuing tests on the HW side at P5.

@fabiocos
Copy link
Contributor

@mmusich @boudoul what is the outcome of the tracker validation in pre6? I was monitoring that to integrate this asap, but I did not see any report in valDB so far

@mmusich
Copy link
Contributor

mmusich commented Jul 13, 2018

@fabiocos, indeed the data report is missing (which is a pity) and the report from @boudoul on simulation (https://hypernews.cern.ch/HyperNews/CMS/get/relval/10839.html) is indeed still open (likely because of many changes introduced in the pixel detector) but a quick look at Strip workspace pointed in the "work in progress" report (https://goo.gl/ThZvmN) makes me conclude that the outcome is positive (I do not see catastrophically wrong changes).
Also, all the other subsystem that depend on the Strip Tracker (such as Tracking) have signed off the data campaign (https://hypernews.cern.ch/HyperNews/CMS/get/relval/10815/6.html), but I agree it would be better to get the Tracker data report (as it is clean from other changes in conditions that have happened in simulation).

@fabiocos
Copy link
Contributor

+operations

backport from master

@cmsbuild
Copy link
Contributor

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

@fabiocos
Copy link
Contributor

+1

Tracker validation of CMSSW_10_2_0_pre6 is reported ok

@cmsbuild cmsbuild merged commit ba9947a into cms-sw:CMSSW_10_1_X Jul 16, 2018
@boudoul
Copy link
Contributor

boudoul commented Jul 16, 2018

Thanks @fabiocos , do you have an estimate when 10_1_9 will come ? In our case the earlier , the better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants