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

rev 1.0: Post-LS1 CSC integration - updated to new data format CSC packe... #4034

Merged
merged 14 commits into from Jun 13, 2014

Conversation

barvic
Copy link
Contributor

@barvic barvic commented May 28, 2014

rev 1.0: Post-LS1 CSC integration (for CMSSW_7_2_X)

  • updated to new data format CSC packer/unpacker (EventFilter/CSCRawToDigi) and CSC DQM (DQM/CSCMonitorModule).
    Should be validated and merged or rejected together (CSCMonitorModule depends on changes in CSCRawToDigi)

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @barvic for CMSSW_7_2_X.

rev 1.0: Post-LS1 CSC integration - updated to new data format CSC packe...

It involves the following packages:

DQM/CSCMonitorModule
EventFilter/CSCRawToDigi

@thspeer, @StoyanStoynev, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @slava77, @Degano, @ojeda can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

@slava77
Copy link
Contributor

slava77 commented May 28, 2014

Hi Victor, thanks for resubmission in 72X.
Is this reasonably final?

@ptcox
Tim, since we have the packer with the updated format, at some point soon we will need to switch to proper digiToRaw-rawToDigi cycle instead of keeping the sim digis.
Is this already possible with the code in this PR (just drop all post-LS1 muon customs and expect everything to work)?
Please comment

@ptcox
Copy link
Contributor

ptcox commented May 28, 2014

Hi Slava,

I haven’t thought about it, but remember this has new code has not been tested very throughly yet. I also need to get the CSCOfflineMonitor pull’ed and integrated so the DQM output for monitoring digis and local reco is included. That should be by the end of the week.

I don’t think there’s anything to be done in Simulation workflows other than to remove the skipping of Digi->Raw and Raw->Digi, but that is NOT ‘all’ PostLS1 customizations (those include unganging ME1/1A, adding the conditions indexing for all new channels, etc.)

Regards, Tim

On May 28, 2014, at 15:46, Slava Krutelyov notifications@github.com wrote:

Hi Victor, thanks for resubmission in 72X.
Is this reasonably final?

@ptcox
Tim, since we have the packer with the updated format, at some point soon we will need to switch to proper digiToRaw-rawToDigi cycle instead of keeping the sim digis.
Is this already possible with the code in this PR (just drop all post-LS1 muon customs and expect everything to work)?
Please comment


Reply to this email directly or view it on GitHub.

ktf added 2 commits May 29, 2014 14:09
Added tryToGet method to EventSetup and EventSetupRecord.
@cmsbuild
Copy link
Contributor

-1
I found an error when building:

>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-05-30-0200/src/EventFilter/CSCRawToDigi/src/CSCAnodeData.cc 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-05-30-0200/src/EventFilter/CSCRawToDigi/src/CSCAnodeData2006.cc 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-05-30-0200/src/EventFilter/CSCRawToDigi/src/CSCAnodeData2007.cc 
In file included from /build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-05-30-0200/src/EventFilter/CSCRawToDigi/src/CSCALCTTrailer.cc:5:0:
/build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-05-30-0200/src/EventFilter/CSCRawToDigi/interface/CSCALCTTrailer.h: In member function 'short unsigned int\* CSCALCTTrailer::data()':
/build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-05-30-0200/src/EventFilter/CSCRawToDigi/interface/CSCALCTTrailer.h:53:29: error: request for member 'load' in 'CSCALCTTrailer::firmwareVersion', which is of non-class type 'short unsigned int'
     switch (firmwareVersion.load()) {
                             ^
/build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-05-30-0200/src/EventFilter/CSCRawToDigi/interface/CSCALCTTrailer.h: In member function 'int CSCALCTTrailer::getCRC()':
/build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-05-30-0200/src/EventFilter/CSCRawToDigi/interface/CSCALCTTrailer.h:88:29: error: request for member 'load' in 'CSCALCTTrailer::firmwareVersion', which is of non-class type 'short unsigned int'
     switch (firmwareVersion.load()) {


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4034/29/summary.html

@ktf
Copy link
Contributor

ktf commented May 30, 2014

Please rebase on the latest IB. firmwareVersion to an std::atomic due to multithreading, but apparently you reversed that change.

@barvic
Copy link
Contributor Author

barvic commented May 30, 2014

Hi Giulio,
I have committed corresponding changes (statics to atomics) to this branch.
There are added pre-processor #ifdefs, which are needed to compile this module outside of CMSSW.

@cmsbuild
Copy link
Contributor

Pull request #4034 was updated. @ojeda, @StoyanStoynev, @danduggan, @rovere, @monttj, @cmsbuild, @nclopezo, @deguio, @slava77, @vadler, @Degano can you please check and sign again.

@cmsbuild
Copy link
Contributor

@deguio
Copy link
Contributor

deguio commented Jun 12, 2014

+1
hello @barvic
I have crosschecked what you say printing the BT for the histo booking and I confirm that the actual booking is performed in the beginRun.
thanks,
F.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jun 12, 2014

+1

for #4034 3a77593

compared to the previously signed version, this changes only DQM/CSCMonitorModule

@slava77
Copy link
Contributor

slava77 commented Jun 12, 2014

@ktf

This PR Changes only in EventFilter/CSCRawToDigi/ and DQM/CSCMonitorModule, analysis signature is not needed. Please bypass it

@barvic
Copy link
Contributor Author

barvic commented Jun 12, 2014

Thanks Federico, Slava

Slava, thanks for clarifying the analysis signature point. I wasn't sure who I need to ask to check and sign this PR.

@ktf
Copy link
Contributor

ktf commented Jun 13, 2014

Sorry, I missed this one.

The reason why it requires AT signature is that in some update new changes to CommonTools/UtilAlgos got introduced and the script is smart enough to notice that (but not to notify about it). I'll bypass and see what can be done to make the bot more clear about new packages introduced by subsequent commits.

ktf added a commit that referenced this pull request Jun 13, 2014
Post-LS1 CSC integration - updated to new data format CSC packer / unpacker
@ktf ktf merged commit 29836fe into cms-sw:CMSSW_7_2_X Jun 13, 2014
@slava77
Copy link
Contributor

slava77 commented Jun 13, 2014

Hi Giulio,

I'm not sure which update you are referring to in CommonTools/UtilAlgos, the actual one (would appear if you try to merge this PR) or the bogus one (when web diff shows a change because it may have failed to resolve changes from merges etc).
It would be the best if bot was checking the actual stuff, but I guess it's a different technology.

makortel added a commit to makortel/cmssw that referenced this pull request Apr 26, 2019
Doing a memcpy over boost::shared_ptr<> makes no sense. From history
these constructors look like a leftover from
a5217e8 of PR cms-sw#4034.
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