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

Remove exceptions from the strip unpacker #28756

Merged
merged 33 commits into from
Jan 29, 2020

Conversation

pieterdavid
Copy link
Contributor

PR description:

These changes address #24825 (also tracked in jira as CMSTRACK-161): the strip unpacker does not throw exceptions anymore (those left in EventFilter/SiStripRawToDigi are only used in the packer, and signal a bad input, so I think they are acceptable). Instead, status codes are returned.
The extra information that used to be in the exception message is printed through LogDebug, so it won't be there in release builds anymore, but the warnings (by default: maximally once per type, and then a count at the end of the job) that were printed are still there (minus the information that came from the exception message - FED and channel ID are still there, these are the most relevant anyway).

Most potential exceptions came from the sistrip::FEDBuffer (and sistrip::FEDSpyBuffer, together with their common base class sistrip::FEDBufferBase) constructor, so I moved the checks this does into a separate method that should be called before constructing a FED(Spy)Buffer, and returns a status code. For the FEDBuffer class there is also an initialisation step that could throw; I made it public and return a status code as well; a typical FEDBuffer construction then looks like this:

const auto st_buffer = sistrip::preconstructCheckFEDBuffer(input);
// check st_buffer
sistrip::FEDBuffer buffer{input};
const auto st_chan = buffer.findChannels();
// check st_chan

see the AlignmentEventFilter diff for a full example (behaviour should be the same as before for this, except for the exception message) - this is also mentioned in the methods' documentation now.

Since I had to change the operator++() functions of the channel unpackers (from where some exceptions could be thrown), and these were all used from a loop like this:

while ( unpacker.hasData() ) {
  output.push_back(...); // (raw) digi with unpacker adc (and strip, for ZS)
  unpacker++;
}

I changed them into methods (see here) that take an output iterator instead, and unpack the whole channel (this allows for a bit of simplification, and makes the code more readable; this was already all inlined before, so I don't expect significant changes in CPU performance, but in principle a few more things could be optimised by the compiler now)1.
With this, I could also move the code that select the correct unpacking mode depending on the readout mode and packet code into a helper method (the earlier changes in #24339 to the regional unpacker already went in this direction), that is shared between the "full" and "regional" unpacker (as a side-effect, the latter now supports all modes, except scope mode, which is only used for calibration, as far as I know), and quite a bit of code could be removed from these plugins.

These changes are purely technical, no changes are expected (also not on the computing performance; most of the unpacker time should be spent in allocating memory for all the digis, and the way that is done is not changed by this PR).

PR validation:

I took 100 events from a file taken in virgin raw mode (2015 HI, with 10bits packing), unpacked and saved all the raw digis, with the unmodified release. Then I ran, for all VR modes, a packer+unpacker combination on top of those and compared the raw digis before and after; likewise for the ZS modes, after running the zero-suppression - no unexpected differences (config).
runTheMatrix.py -l 140.55 (2018 HI hybrid emulation + repacking + reco workflow, this is one of the more exotic unpacker use cases)
If anyone knows of data with problems that should trigger one of these exceptions-turned-warnings, that would be very useful to test as well.

CC: @robervalwalsh @mmusich @tsusa @alesaggio


1 in passing I made a few small changes to SiStripRawDigi (which is nothing more than a uint16_t that inherits from edm::DoNotSortUponInsertion) - the one that was convenient is conversion back to uint16_t, but I think the other ones are sensible and binary compatible (I don't know of anywhere these are persisted anyway).

…o do the checks upfront (and reorganise to share the implementation)
@cmsbuild
Copy link
Contributor

+1
Tested at: f0c4e61
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5e6c19/4314/summary.html
CMSSW: CMSSW_11_1_X_2020-01-22-1100
SCRAM_ARCH: slc7_amd64_gcc820

@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-5e6c19/4314/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2697090
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2696743
  • DQMHistoTests: Total skipped: 346
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@jfernan2
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Jan 22, 2020

+1

for #28756 f0c4e61

  • code changes are in line with the PR description and the follow up review.
  • jenkins tests pass and comparisons with the baseline show no differences, as expected from this mainly technical change

@civanch
Copy link
Contributor

civanch commented Jan 22, 2020

+1

@slava77
Copy link
Contributor

slava77 commented Jan 27, 2020

@christopheralanwest @tocheng we need an alca signature for this PR.
Please check and comment if there are issues.
Thank you.

@silviodonato
Copy link
Contributor

This is just a friendly reminder to @christopheralanwest @tocheng

@christopheralanwest
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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. @davidlange6, @silviodonato, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

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.

9 participants