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

CTPPS: new CondFormats related to TOTEM RP raw data and digi #13838

Merged
merged 4 commits into from Apr 19, 2016

Conversation

jan-kaspar
Copy link
Contributor

2nd pull request out of 3 related to TOTEM RP raw-data unpacking and raw-to-digi conversion.

Validation config included in the 3rd pull request (EventFilter).

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jan-kaspar for CMSSW_8_1_X.

It involves the following packages:

CondFormats/DataRecord
CondFormats/TotemReadoutObjects
DataFormats/TotemRPDetId
DataFormats/TotemRPDigi
DataFormats/TotemRPL1
DataFormats/TotemRawData

The following packages do not have a category, yet:

CondFormats/TotemReadoutObjects
DataFormats/TotemRPDetId
DataFormats/TotemRPDigi
DataFormats/TotemRPL1
DataFormats/TotemRawData

@cerminar, @cmsbuild, @franzoni, @ggovi, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@apfeiffer1, @tocheng this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@mmusich
Copy link
Contributor

mmusich commented Mar 25, 2016

please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Mar 26, 2016

@mmusich
once we confirm CondFormats/TotemReadoutObjects to be an OK name for the package,
please edit categories.py file in cms-sw/cms-bot (can edit online and submit a PR based on that edit).

#ifndef _TotemDAQMapping_h_
#define _TotemDAQMapping_h_

#include "DataFormats/TotemRawData/interface/TotemFramePosition.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

TotemFramePosition looks like the only "DataFromats/" object included here.
Looking at it, the class describes the hardware channel ID, something that is fixed for that geometry or logical hardware element and doesn't change between events.
In other detectors we have such objects in CondFormats/[DetectorName]Objects

@slava77
Copy link
Contributor

slava77 commented Mar 26, 2016

@jan-kaspar
In the tests summary #13838 (comment)
you can see in the "Static checks outputs" that there are some issues with the code
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13838/12065/llvm-analysis/
these better be addressed as well

@jan-kaspar
Copy link
Contributor Author

@slava77 Looking at the "Static checks outputs" I have some questions.

  • Is only catch (...) forbidden or any kind of catch?
  • Is the problem with inheriting from edm:EDAnalzer that now you prefer to inherit from edm::one::EDAnalyzer<> ?

Thanks!

@slava77
Copy link
Contributor

slava77 commented Mar 26, 2016

On 3/26/16 1:24 PM, jan-kaspar wrote:

@slava77 https://github.com/slava77 Looking at the "Static checks
outputs" I have some questions.

Is only |catch (...)| forbidden or any kind of |catch|?

Exception throw and catch can not be used to communicate between pieces
of code that we control.
XercesDOMParser is an external.
If we know types of exceptions it can throw it may be better to replace
the "...".

Here you rethrow with a more useful message.
I think that it should be OK to catch

Is the problem with inheriting from |edm:EDAnalzer| that now you
prefer to inherit from |edm::one::EDAnalyzer<>| ?

edm::one looks good for this.

Thanks!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#13838 (comment)

@slava77
Copy link
Contributor

slava77 commented Mar 29, 2016

something to get cms-bot to update the signatures

@cmsbuild
Copy link
Contributor

Pull request #13838 was updated. @civanch, @cvuosalo, @mdhildreth, @franzoni, @cerminar, @slava77, @ggovi, @mmusich, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Mar 31, 2016

cms-bot : update the labels

@slava77
Copy link
Contributor

slava77 commented Apr 1, 2016

@mmusich @franzoni @ggovi
please clarify on the status of review of files in CondFormats.
There were no comments from alca or db for 7 days already.
Thank you.

@mmusich
Copy link
Contributor

mmusich commented Apr 1, 2016

@slava77 thanks for the heads up, we are in the process of reviewing. from a quick view looks like CondFormats are not mapped to Records @ggovi to comment more.

@cmsbuild
Copy link
Contributor

Pull request #13838 was updated. @cerminar, @cmsbuild, @franzoni, @ggovi, @mmusich, @davidlange6 can you please check and sign again.

@jan-kaspar
Copy link
Contributor Author

Sorry, the commit adc852b was done in error.

The merge conflicts should be now resolved.

@slava77
Copy link
Contributor

slava77 commented Apr 14, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@ggovi
Copy link
Contributor

ggovi commented Apr 15, 2016

+1

@davidlange6
Copy link
Contributor

@franzoni @mmusich - any comments on this PR ? we should merge it today if no further questions

@mmusich
Copy link
Contributor

mmusich commented Apr 19, 2016

@davidlange6 I think after talking with CT-PPS representatives last Friday we decided to ease integration by merging this first.

@mmusich
Copy link
Contributor

mmusich commented Apr 19, 2016

+1

@davidlange6 davidlange6 merged commit 221e750 into cms-sw:CMSSW_8_1_X Apr 19, 2016
@jan-kaspar jan-kaspar deleted the ctpps_raw_cond_formats branch April 19, 2016 08:50
davidlange6 added a commit that referenced this pull request May 27, 2016
CTPPS: new CondFormats related to TOTEM RP raw data and digi - backport of #13838
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

7 participants