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 DataFormats related to TOTEM RP raw data and digi #13837

Merged
merged 6 commits into from Apr 14, 2016

Conversation

jan-kaspar
Copy link
Contributor

1st 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:

DataFormats/TotemRPDetId
DataFormats/TotemRPDigi
DataFormats/TotemRPL1
DataFormats/TotemRawData

The following packages do not have a category, yet:

DataFormats/TotemRPDetId
DataFormats/TotemRPDigi
DataFormats/TotemRPL1
DataFormats/TotemRawData

@cmsbuild, @davidlange6 can you please review it and eventually sign? Thanks.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@slava77
Copy link
Contributor

slava77 commented Mar 26, 2016

  • TotemRPL1 should be renamed to L1TotemRP. See the patterns in https://github.com/cms-sw/cmssw/tree/CMSSW_8_0_X/DataFormats
  • classes internal to Raw2DigiProducer should probably stay in the same package adn the producer itself DataFormats/TotemRawData. If we have digis, I don't see a good reason to keep them in DataFormats/ with objects intended to be saved in standard output data streams/datasets.
  • classes_def.xml should have class versions (start from 2) together with checksums for all basic classes (not templated or derived from templates)
  • CMS style is to start with lower case for all class methods (excluding constructors and destructors). E.g. e.g. in TotRPDetId.h int Station() should be int station()

@slava77
Copy link
Contributor

slava77 commented Mar 26, 2016

@cmsbuild please test
to see if more issues come up in compilation

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@jan-kaspar
Copy link
Contributor Author

@slava77

TotemRPL1 should be renamed to L1TotemRP. See the patterns in https://github.com/cms-sw/cmssw/tree/CMSSW_8_0_X/DataFormats

OK

classes internal to Raw2DigiProducer should probably stay in the same package adn the producer itself DataFormats/TotemRawData. If we have digis, I don't see a good reason to keep them in DataFormats/ with objects intended to be saved in standard output data streams/datasets.

I am not sure whether I understand. DataFormats/TotemRawData contains non-digi data that we save in Event:

  • TotemRawEvent: various counters from DAQ and Trigger
  • TotemRawToDigiStatus: status of raw-to-digi conversion for each read-out chip (VFAT), this was important for some analyses

classes_def.xml should have class versions (start from 2) together with checksums for all basic classes (not templated or derived from templates)

OK

CMS style is to start with lower case for all class methods (excluding constructors and destructors). E.g. e.g. in TotRPDetId.h int Station() should be int station()

How strict is this requirement? Unfortunately, most of our code has method names starting with a capital...

@slava77
Copy link
Contributor

slava77 commented Mar 27, 2016

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

CMS style is to start with lower case for all class methods
(excluding constructors and destructors). E.g. e.g. in TotRPDetId.h
int Station() should be int station()

How strict is this requirement? Unfortunately, most of our code has
method names starting with a capital...

Consistency in the data formats method naming is more important that in
the algorithms.
Isn't it just a search-and-replace, which can probably be sed scripted

@@ -0,0 +1,6 @@
<use name="DataFormats/DetId"/>
<use name="FWCore/Utilities"/>
<use name="rootrflx"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not necessary anymore

/**
* Trigger counters from LoneG.
**/
class TotemTriggerCounters
Copy link
Contributor

Choose a reason for hiding this comment

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

you can go back to struct.
class with public data member is worse.

@cmsbuild
Copy link
Contributor

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

@jan-kaspar
Copy link
Contributor Author

@slava77 Struct is back.

@civanch
Copy link
Contributor

civanch commented Apr 13, 2016

please test

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Apr 13, 2016

+1

for #13837 b836b2a

  • the last update adds a default initialization for TotemTriggerCounters.h struct. Compilation is enough (jenkins is done compiling by now)

@cmsbuild
Copy link
Contributor

@civanch
Copy link
Contributor

civanch commented Apr 13, 2016

+1

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit dc1314b into cms-sw:CMSSW_8_1_X Apr 14, 2016
@jan-kaspar jan-kaspar deleted the ctpps_raw_data_formats branch April 19, 2016 08:51
cmsbuild added a commit that referenced this pull request May 24, 2016
CTPPS: new DataFormats related to TOTEM RP raw data and digi - backport of PR #13837
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

8 participants