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
Clean-up CSC Examiner related code and configs #24318
Comments
assign reconstruction |
A new Issue was created by @perrotta . @davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
The dead assignment of The cleaning of the configs with different ExaminerMask's implemented in them is still pending. According to what was written by @barvic in #24295 (comment):
|
I have to say I do not think it is a smart idea to blithely delete
complex code like this. You have a lot more confidence than me that such
code will never need to be reimplemented. And likewise with old config
files. To me it seems better to leave them around because they show what
once WORKED even if they no longer do. Better to comment out code that
is unused but now troublesome rather than completely erase it.
Tim
perrotta wrote on 5/13/19 11:35:
…
The dead assignment of |fTMB_Scope| and related flags was fixed by
#26574 <#26574>
The cleaning of the configs with different ExaminerMask's implemented
in them is still pending. According to what was written by @barvic
<https://github.com/barvic> in #24295 (comment)
<#24295 (comment)>:
* Those test config files with .cfg extensions are obsolete and
unusable (from pre-python era). Most likely will be just removed
for general clean up purposes at some point
* Those in DQM looks like old L1T monitoring-related and in
principle it would be better to be updated as well for
consistency, but I think that particular DQM module is obsolete in
Run2
@ptcox <https://github.com/ptcox>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24318 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABGYLHWPDAD3AI7UJT4UPB3PVEY4NANCNFSM4FQACMUA>.
|
@ptcox old code is always available in the git history in case it needs to be revived. Leaving large chunks of commented-out code (or completely broken configs) is discouraged because it is messy and confusing for any future observer or developer. |
I find untangling old code from git history next to impossible. And that
is only feasible anyway if somebody KNOWS that some particular feature
was once implemented and then deleted. The same is the case for deleted
config files. Their existence, even if they no longer operate, can
illuminate the fact that once some particular feature or set of features
WAS operational, information that can otherwise get lost to
institutional memory. I do not consider this as trivial as apparently
you do.
Kevin Pedro wrote on 5/13/19 15:48:
…
@ptcox <https://github.com/ptcox> old code is always available in the
git history in case it needs to be revived. Leaving large chunks of
commented-out code (or completely broken configs) is discouraged
because it is messy and confusing for any future observer or developer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24318?email_source=notifications&email_token=ABGYLHUAXWRI5B3JNUAFLXTPVFWTZA5CNFSM4FQACMUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVILTPI#issuecomment-491829693>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABGYLHT3ZYSQ2NTXKJWK4YLPVFWTZANCNFSM4FQACMUA>.
|
@ptcox this is one of the primary purposes of version control systems. There are numerous resources available to become more familiar with Git, including a tutorial I wrote specifically for CMS: https://twiki.cern.ch/twiki/bin/view/CMS/CMSGitTutorial. Additionally, we usually clean up old code only in the development branch. All the previous branches, corresponding to older release cycles of CMSSW, will still have the old code present, so it is quite easy to locate it (without e.g. memorizing some commit hash). |
While reviewing #24295 it was noticed that (see #24295 (comment) and #24295 (comment)):
There are several (test and DQM) configs which define different ExaminerMask's:
http://cmslxr.fnal.gov/search?_filestring=&_string=ExaminerMask&_casesensitive=1
Those masks are ideally supposed to be the same. The problem is that that most of those configs are obsolete, and need to be removed from the repository (or alternatively brought up to date)
There is a fTMB_Scope flag in the CSCDCCExaminer which was originally supposed to be for a handling of the debugging mode feature of CSC hardware, which is not used in the production. It being defined and never used generates a complain in the static analyzer. The code in /EventFilter/CSCRawToDigi/src/CSCDCCExaminer.cc must be cleaned up of it, and of whatever else obsolete is in it
@barvic @Martin-Grunewald @slava77
The text was updated successfully, but these errors were encountered: