CMS code rules for CSC digi and trigger objects #29782
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29782/15239
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29782/15255
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29782/15264
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29782/15265
|
|
A new Pull Request was created by @dildick (Sven Dildick) for master. It involves the following packages: DQM/CSCMonitorModule @perrotta, @benkrikler, @kmaeshima, @civanch, @schneiml, @andrius-k, @mdhildreth, @cmsbuild, @rekovic, @jfernan2, @fioriNTU, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@dildick there seems to be CMSSW code rule errors: |
| MonitorElement *hALCTgetBX2DNumerator; | ||
| MonitorElement *hALCTbx; | ||
| MonitorElement *hALCTbxSerial; | ||
| // MonitorElement *hALCTbxChamberMeans; |
There was a problem hiding this comment.
These name changes will probably need a PR in
https://github.com/dmwm/deployment/blob/master/dqmgui/style/CSCRenderPlugin.cc#L2684-L2686
to mantain the same style in DQM GUI
There was a problem hiding this comment.
@jfernan2 I'll undo the changes in the histogram naming. Thanks for pointing this out.
| @@ -185,40 +185,40 @@ class CSCCorrelatedLCTDigi { | |||
| // more information, please check "DN-20-016". | |||
|
|
|||
| // Run-1, Run-2 and Run-3 trknmb is either 1 or 2. | |||
| uint16_t trknmb; | |||
| uint16_t trackNumber_; | |||
There was a problem hiding this comment.
is there any old data where CSCCorrelatedLCTDigi should still be readable correctly?
I do not see any ioread rules to preserve backward compatibility.
There was a problem hiding this comment.
Hi Slava, yes, I need to add the ioread rules. Thanks.
|
This looks scary. People doing Tag & Probe for CSC LCT efficiencies
expect to be able to read CorrelatedLCT digis in ALL data that ever
existed. Of course, perhaps these specific members aren't used, but I
have no idea.
Tim
… Slava Krutelyov ***@***.***>
May 11, 2020 at 16:09
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In DataFormats/CSCDigi/interface/CSCCorrelatedLCTDigi.h
<#29782 (comment)>:
> @@ -185,40 +185,40 @@ class CSCCorrelatedLCTDigi {
// more information, please check "DN-20-016".
// Run-1, Run-2 and Run-3 trknmb is either 1 or 2.
- uint16_t trknmb;
+ uint16_t trackNumber_;
is there any old data where CSCCorrelatedLCTDigi should still be
readable correctly?
I do not see any ioread rules to preserve backward compatibility.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29782 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGYLHSTBFAX5RTG4O6XURLRRABJ3ANCNFSM4M4JXX4Q>.
|
is there an example workflow to check? |
|
No workflow - the code is entirely outside CMSSW as work in progress:
https://github.com/stremreich/CSCefficiency/tree/master/CSCEfficiency
I don't understand what you mean by 'main cause of concern is reading
the sim digis', or 'The regular digis are typically repacked, where the
compatibility is ensured at the FED RAW level.'
The CSCCorrelatedLCTDigi should not get changed in any
backward-incompatible way because then it will be a big and unwanted
task for people to have rewrite all the T&P code (which is hard enough
to follow as it is). This includes not changing the names of functions
in the class, of course. I'm sorry - I haven't been closely following
with what changes are being made to CSC digis but any potential changes
should be presented to CSC first, and agreement on any modifications
first obtained from them. At least that way we can expect to see who
might be affected by such changes.
Perhaps I am just failing to understand what is being changed, and all
will be well?
Regards, Tim
… Slava Krutelyov ***@***.***>
May 11, 2020 at 17:16
is there an example workflow to check?
I suppose that the main case of concern is reading the sim digis of
type CSCCorrelatedLCTDigi. The regular digis are typically repacked,
where the compatibility is ensured at the FED RAW level.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29782 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGYLHQJC4AI7GSXNH2I4XDRRAJDTANCNFSM4M4JXX4Q>.
|
|
@ptcox I understand your concerns. The T&P studies on CSC LCTs are particularly important. I'm fine with delaying this for a few months and closing this PR for now. |
dildick
deleted the
from-CMSSW_11_1_X_2020-05-08-1100-CMS-code-rules-CSC-trigger_v2
branch
PR description:
Apply section 2.9 and 2.11 of http://cms-sw.github.io/cms_coding_rules.html onto several CSC digi and trigger objects, per Vladimir Rekovic's request (#29456 (comment)). For nearly all changes I ran a recursive search-replace.
I renamed
CSCCLCTDatainEventFilter/CSCRawToDigitoCSCComparatorData, because it does not actually contain the CLCTs. It only contains comparator digis. In fact, CLCTs are kept inCSCTMBHeaderI removed
CSCComparatorDigiFitterinL1Trigger/CSCTriggerPrimitives. It was originally meant to be used in the CLCT processor, but it was never properly integrated, and the CCLUT algorithm will be doing that in the near future (see various PRs with tagCCLUT).PR validation:
Code compiles. Tested with WF 26634.0.
if this PR is a backport please specify the original PR and why you need to backport that PR:
N/A