cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.08k stars 4.29k forks source link

CondFormats dependencies in DataFormats #30189

Open davidlange6 opened 4 years ago

davidlange6 commented 4 years ago

I'd like to remove the dependency of DataFormats/PatCandidates on CondFormats/L1TObjects (which I believe are not supposed to be allowed...).

This can be accomplished by moving two enums from CondFormats/L1TObjects/interface/L1GtDefinitions.h to some DataFormats package. (presumably DataFormats/L1TGlobal). Are there reasons not to do this or to do it differently?

cmsbuild commented 4 years ago

A new Issue was created by @davidlange6 David Lange.

@Dr15Jones, @silviodonato, @dpiparo, @smuzaffar, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

davidlange6 commented 4 years ago

assign l1

cmsbuild commented 4 years ago

New categories assigned: l1

@benkrikler,@rekovic you have been requested to review this Pull request/Issue and eventually sign? Thanks

davidlange6 commented 4 years ago

assign core

cmsbuild commented 4 years ago

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

smuzaffar commented 4 years ago

sounds reasonable to me.

makortel commented 4 years ago

Sounds good to me too.

guitargeek commented 4 years ago

A related issue might be the L1Trigger dependency in DataFormats (it's just this single one): https://github.com/cms-sw/cmssw/blob/master/DataFormats/L1TMuon/src/EMTFHit.cc#L2

Wouldn't the DataFormats have no dependencies in an ideal world, besides FWCore and maybe CommonTools for tests?

makortel commented 4 years ago

@guitargeek Good catch, that dependence is not allowed (any volunteers to move things around?).

The allowed dependencies on packages defining persisted data formats are listed here https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideCreatingNewProducts#Restrictions_on_the_package_defi (packages defining transient-only products have less constraints)

guitargeek commented 4 years ago

Thanks for the information. I don't know if it's good to move this code around without coordinating with the original developers, because it seems to be actively developed in the last years.

However, if I were to do it I would just move all of these member functions implemented in EMTFHit.cc into some new file EventFilter/L1TRawToDigi/plugins/implementations_stage2/EMTFHitFunctions.h (or something), because it's the only place where they are used. All of the member variables of EMTFHit are public, so it should be no problem to turn the member functions into free functions.

davidlange6 commented 4 years ago

more fun are the CondFormats dependencies in DataFormats/RPCDigi. If one wants to make a class that is both storable in the event and storable in the conditions db, what is the design for that? (or is there one?)

davidlange6 commented 4 years ago

I've proposed L1 changes to #31346

davidlange6 commented 3 years ago

Ping about DataFormats/RPCDigi.

cecilecaillol commented 2 years ago

Addressed in https://github.com/cms-sw/cmssw/commit/c13ac4554e31202e760920b0a3f63d31ed29b749

cecilecaillol commented 2 years ago

+l1