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

Updated SiStrip unpacker (new FED readout modes) #12387

Merged
merged 1 commit into from
Nov 16, 2015

Conversation

forthommel
Copy link
Contributor

Introducing changes from #12157 and #12385 (from 7_5_X) to the 8_0_X release to be able to unpack new data formats introduced in the last FED FW upgrade (see these two PRs for more details)

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

DataFormats/SiStripCommon
EventFilter/SiStripRawToDigi
RecoLocalTracker/SiStripClusterizer

@cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @makortel, @yduhm, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @nickmccoll, @jlagram, @gpetruc, @cerati, @threus this is something you requested to watch as well.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/L2's to mark it as on hold
  • merge: L1 to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9662/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 12, 2015

apparently, this one still merges OK in 80X after #12307 is merged.
Nice.

sistrip::FEDZSChannelUnpacker unpacker = sistrip::FEDZSChannelUnpacker::zeroSuppressedLiteModeUnpacker(buffer->channel(fedCh));

// unpack
clusterizer.addFed(unpacker,ipair,record);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line wil not compile anymore...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot reproduce on my test bench. Could you please give me your compiler output?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to merge PR #12307
if you collapse Lite8 and lite10 there will be no problem..

@slava77
Copy link
Contributor

slava77 commented Nov 12, 2015

@forthommel
please remove the merge of 12307, it's already in the 80X branch.
Either rebase to CMSSW_8_0_X_2015-11-12-1100 or remake in this IB from scratch.

@cmsbuild
Copy link
Contributor

Pull request #12387 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

@forthommel forthommel force-pushed the newSiStripFEDUnpacker branch from 63eeca4 to d084963 Compare November 12, 2015 19:48
@slava77
Copy link
Contributor

slava77 commented Nov 13, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9714/console

@cmsbuild
Copy link
Contributor

Pull request #12387 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 15, 2015

+1

for #12387 d084963

  • code changes look consistent with 75X/76X modifications; although the cleanup made in the processing options as seen in this PR and not available in 76X/75X (see below) could have been propagated back to 76X/75X
  • jenkins tests pass and comparisons with the baseline show no differences

comparison of "76X version (#12161 fb69762) " .. "80X version (this PR)" (these could have been added back to 76X)

--- a/RecoLocalTracker/SiStripClusterizer/plugins/ClustersFromRawProducer.cc
+++ b/RecoLocalTracker/SiStripClusterizer/plugins/ClustersFromRawProducer.cc
@@ -346,7 +346,7 @@ try { // edmNew::CapacityExaustedException
     const sistrip::FEDReadoutMode mode = buffer->readoutMode();


-    if likely(mode == sistrip::READOUT_MODE_ZERO_SUPPRESSED_LITE10 ) { 
+    if likely(mode == sistrip::READOUT_MODE_ZERO_SUPPRESSED_LITE10 || mode == sistrip::READOUT_MODE_ZERO_SUPPRESSED_LITE8) { 

        try {
          // create unpacker
@@ -370,28 +370,9 @@ try { // edmNew::CapacityExaustedException
          }                                               
          continue;
        }
-    } else if likely(mode == sistrip::READOUT_MODE_ZERO_SUPPRESSED_LITE8 ) {
-
-      try {
-        // create unpacker
-        sistrip::FEDZSChannelUnpacker unpacker = sistrip::FEDZSChannelUnpacker::zeroSuppressedLiteModeUnpacker(buffer->channel(fedCh));
-
-        // unpack
-        clusterizer.addFed(unpacker,ipair,record);
-      } catch (edmNew::CapacityExaustedException) {
-        throw;
-      } catch (const cms::Exception& e) {
-          if (edm::isDebugEnabled()) {
-            std::ostringstream ss;
-            ss << "Unordered clusters for channel " << fedCh << " on FED " << fedId << ": " << e.what();
-            edm::LogWarning(sistrip::mlRawToCluster_) << ss.str();
-          }
-          continue;
-        }
-
     } else {

-      if (mode == sistrip::READOUT_MODE_ZERO_SUPPRESSED or mode == sistrip::READOUT_MODE_ZERO_SUPPRESSED_FAKE ) { 
+      if (mode == sistrip::READOUT_MODE_ZERO_SUPPRESSED || mode == sistrip::READOUT_MODE_ZERO_SUPPRESSED_FAKE ) { 
        try {
          // create unpacker
          sistrip::FEDZSChannelUnpacker unpacker = sistrip::FEDZSChannelUnpacker::zeroSuppressedModeUnpacker(buffer->channel(fedCh));

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Nov 16, 2015
Updated SiStrip unpacker (new FED readout modes)
@cmsbuild cmsbuild merged commit 57099d7 into cms-sw:CMSSW_8_0_X Nov 16, 2015
@Martin-Grunewald
Copy link
Contributor

Can you please clarify the value to be used for LegacyUnpacker:
For Run-1 data it is TRUE, for Run-2 data it is FALSE?
Or is it true for all data taken up to now, and false only for future 2016+ data?
How is it the case with MC?

@slava77
Copy link
Contributor

slava77 commented Dec 17, 2015

IIRC, LegacyUnpacker=true is introduced mostly for safety. The value "false" so far appeared to work OK for all.
@forthommel should confirm.

@forthommel forthommel deleted the newSiStripFEDUnpacker branch July 17, 2017 13:04
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.

7 participants