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

ZDC unpacker update URGENT #12544

Merged
merged 6 commits into from Nov 24, 2015
Merged

Conversation

BetterWang
Copy link
Contributor

[+] update working on compact FEDs readout

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @BetterWang (Quan Wang) for CMSSW_7_5_X.

It involves the following packages:

EventFilter/CastorRawToDigi

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

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/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers 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

@BetterWang
Copy link
Contributor Author

Hi, @davidlange6 @slava77 @Degano @smuzaffar , can anyone of you trigger the test?
This is an urgent PR to fix the ZDC unpacker needed for the heavy ion run.
Thanks,

@slava77
Copy link
Contributor

slava77 commented Nov 23, 2015

@BetterWang please describe the impact of the changes, including why the old one doesn't work and also provide some information showing that this update fixes particular ptoblems.

The last iteration was told to be well debugged and now we get a 500 lines worth of bugfixes.
This doesn't quite make sense.

@slava77
Copy link
Contributor

slava77 commented Nov 23, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@BetterWang
Copy link
Contributor Author

Dear @slava77 , the last update was done based on the old FED format from 2011 PbPb run, which has been updated since then.
Now, this unpacker works fine with the new data format. However, it doesn't handle the old data.
The old data will still be handled by Hcal unpacker.
Now, I have a question. How do we make the zdcreco picking up the new label "castorDigis" instead of the old "hcalDigis", for data taking?
I haven't change any labels since it will probably cause missing label error in the matrix.
If you prefer, we can talk on skype or phone. I sent you an email just now.
Thanks,

@@ -21,7 +21,7 @@ using namespace std;
CastorRawToDigi::CastorRawToDigi(edm::ParameterSet const& conf):
dataTag_(conf.getParameter<edm::InputTag>("InputLabel")),
unpacker_(conf.getParameter<int>("CastorFirstFED"),conf.getParameter<int>("firstSample"),conf.getParameter<int>("lastSample")),
zdcunpacker_(conf.getParameter<int>("ZDCFirstFED"),conf.getParameter<int>("firstSample"),conf.getParameter<int>("lastSample")),
zdcunpacker_(conf.getParameter<int>("CastorFirstFED"),conf.getParameter<int>("firstSample"),conf.getParameter<int>("lastSample")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct?
it looks like the variable is actually not used (but if it would be used, it seems like the parameter would be wrong)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used as sourceIdOffset_:
https://github.com/BetterWang/cmssw/blob/ZDCunpacker_update/EventFilter/CastorRawToDigi/src/ZdcUnpacker.cc#L84
This is because the FED index is with respect to the first FED number.
In this case, the actual ZDC FED is 693. When referred in the ElectronicId, the uccid=693-690=3, where 690 is the FED number of the first Castor FED.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the first argument in the ZdcUnpacker ctor.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I know that, but the data member set by this argument is not used anywhere in ZdcUnpacker.cc

@BetterWang
Copy link
Contributor Author

Hi all, you can find the slides at
https://www.dropbox.com/s/jpb9tlf2sqta5ju/11-23-15_ZDCunpacker.pdf?dl=0
Thanks,

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Nov 23, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

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

@@ -10,8 +10,7 @@

RecoHiEvtPlaneAOD = cms.PSet(
outputCommands = cms.untracked.vstring('keep recoEvtPlanes_hiEvtPlane_*_*',
'keep ZDCRecHitsSorted_zdcreco_*_*',
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you drop the hits?
the keep statement should be here (despite the fact that currently these hits don't make much sense for run2 data/data unpacked into castorDigis)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, putting it back.

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Nov 23, 2015

@cmsbuild please test
[the third time might do it]

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

-1
Tested at: 4affe83
The relvals timed out after 2 hours.

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

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 23, 2015

@smuzaffar @Degano
please check the logic in the bot for error (tests-rejected assignment): the failed test was for an older commit, while the latest commit is OK. So, technically, the label should be "tests-approved" corresponding to 75fd713

@cmsbuild cmsbuild merged commit 75fd713 into cms-sw:CMSSW_7_5_X Nov 24, 2015
cmsbuild added a commit that referenced this pull request Nov 24, 2015
…75fd713-fixHIAOD

ZDC unpacker update (as #12544 but also keep old zdc digis in HI AOD)
@smuzaffar
Copy link
Contributor

@slava77, I will update cms-bot to also take account the latest commit hash

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

4 participants