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
Strip Hit Efficiency from DQM: first push #37530
Strip Hit Efficiency from DQM: first push #37530
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37530/29241
|
A new Pull Request was created by @mmusich (Marco Musich) for master. It involves the following packages:
@malbouis, @yuanchao, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @cmsbuild, @jfernan2, @francescobrivio, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
CalibTracker/SiStripHitEfficiency/plugins/SiStripHitEffFromCalibTree.cc
Outdated
Show resolved
Hide resolved
CalibTracker/SiStripHitEfficiency/plugins/SiStripHitEffFromCalibTree.cc
Outdated
Show resolved
Hide resolved
CalibTracker/SiStripHitEfficiency/plugins/SiStripHitEfficiencyHarvester.cc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Marco, All,
This is a nice development plan. I welcome the idea of moving this to DQM/PCL.
I made a few comments, I wasnt sure if you wanted me to point out all occurrences, as of new I only pointed to a few cases
cout
-s should be moved to MsgLogger- there are some cases where
float
could be considered instead ofdouble
- some of the magic numbers should be named constants
- the tokens I think could be
const
-s - in a few cases the
int
could be madeunsigned
CalibTracker/SiStripHitEfficiency/plugins/SiStripHitEfficiencyHarvester.cc
Outdated
Show resolved
Hide resolved
//legend: nBadComp[i][j][k]= SubSystem i, layer/disk/wheel j, BadModule/Fiber/Apv k | ||
// i: 0=TIB, 1=TID, 2=TOB, 3=TEC | ||
// k: 0=BadModule, 1=BadFiber, 2=BadApv, 3=BadStrips | ||
std::stringstream ssV[4][19]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the 4 and the 19 be names constants?
CalibTracker/SiStripHitEfficiency/plugins/SiStripHitEfficiencyWorker.cc
Outdated
Show resolved
Hide resolved
CalibTracker/SiStripHitEfficiency/plugins/SiStripHitEfficiencyHarvester.cc
Outdated
Show resolved
Hide resolved
CalibTracker/SiStripHitEfficiency/plugins/SiStripHitEfficiencyHarvester.cc
Outdated
Show resolved
Hide resolved
please test
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-65994f/23822/summary.html Comparison SummarySummary:
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37530/29257
|
Pull request #37530 was updated. @malbouis, @yuanchao, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @cmsbuild, @jfernan2, @francescobrivio, @micsucmed, @rvenditti can you please check and sign again. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-65994f/23979/summary.html Comparison SummarySummary:
|
+1 |
Hi Marco, given there are virtual contributions in #37530 (comment) , could you please squash the commits that cause them? Thanks! |
I'd rather not, as they would hide the actual main contributor which has left the collaboration. |
Hmmm there are 40 commits changing 18 files. Maybe the author's name could be included in the package header description? @cms-sw/orp-l2 what's your take? |
+alca
|
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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
@cms-sw/orp-l2 what's the integration plan for this PR? Shall I rebase? This is not urgent, so it can also go after 12_4_0_pre3 is built, but I'd just like to understand if more actions are needed on my side |
No strong opinion as for this specific PR. If mainly for historic reason to keep the contributor's name, probably we can keep as it is. |
type trk |
kindly pinging @cms-sw/orp-l2 |
+1 |
PR description:
There is a standing plan to move the strip hit efficiency analysis to DQM (either at PCL, or in standard DQM), so that the creation of custom SiStrip calibration trees could be finally relinquished for Run-3.
This PR collects the commits available so far in order to move on with the integration.
PR validation:
cmssw
compiles, the introduced unit tests inCalibTracker/SiStripHitEfficiency
pass.if this PR is a backport please specify the original PR and why you need to backport that PR:
N.A.
cc:
@robervalwalsh @mdelcourt @sroychow @cms-sw/trk-dpg-l2