-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix the signature of SiStripTrivialClusterSource::beginRun() #25693
Fix the signature of SiStripTrivialClusterSource::beginRun() #25693
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25693/8063
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: EventFilter/SiStripRawToDigi @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please test |
The tests are being triggered in jenkins. |
I'm a bit tempted to ask whether this class is still useful (the last non-technical commit was in 2007)? |
Comparison job queued. |
since we can always revert a remove, why not try it now. it's not used in production (based on git grep) |
Comparison is ready Comparison Summary:
|
Indeed, it seems to be only used for purely technical testing of SiStripFEDRawDataAnalyzer (from this config). To be honest I did not know about this class, for the latest developments we used raw files as a starting point instead. If you think it is best to remove this I won't object, but there is more such test code in the package... I created a task on Jira to review/update that in any case (I will work on this as an EPR task in 2019). |
Thanks for the comments. If you're going to review the contents of the test code in this package, I think it is easiest to leave this class as part of that exercise. |
+1
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
This means that earlier the
beginRun()
was not called by the framework, and as a knock-on effect theproduce()
didn't do much useful work.Tested in CMSSW_10_5_X_2019-01-13-0000.