DQM quality monitor for SiStrip Gain (G2) from Multi Run Harvesting #18531
Conversation
|
A new Pull Request was created by @dimattia for master. It involves the following packages: CalibTracker/SiStripChannelGain @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
The test looks fine. Could this PR be approved? Are we missing something? Thanks, Alessandro. |
|
Hi @dimattia , you should let people the time to review the code- The PR tests are one thing, the code review is another - btw I have a question about this code , remind me, what it the rationale of redefining methods to get the subdetectorID , side, etc , rather than using existing ones ? |
|
Hi @boudoul, these are helper functions that extracts the geometry coordinates from either the detId or the string of char which names an histogram for the monitor. I wanted to have a set of functions completely symmetric for this purpose. |
|
+1 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
point taken, but the purpose of this pull request was to implement the DQM monitoring, not to re-engineer the SiStripGainFromCalibTree. As already stated it will be addressed at a later stage. |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
use of const string& implemented. |
|
@franzoni @arunhep can the tests be restarted? |
|
please test
(but only new code is being reviewed so I don't understand the comment..)
… On Jun 1, 2017, at 9:37 AM, Marco Musich ***@***.***> wrote:
@franozi @arunhep can the tests be restarted?
This PR is mandatory for the monitoring of the calibration workflow and frankly re-adjusting the classes to provide unified helper tools for a class which we intend to decommission seems a waste of developer time.
As for the gigantic TH2S, we are studying how safe it is to lower the number of bins per APV.
Fast feedback is appreciated.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
+1 |
The aim of this Pull Request is to implement the quality monitoring of the cluster charge and of the gain calibration after the multi run haversting step. The monitoring histograms have been implemented in the new code re-engineered to be compliant with the multi-thread environment (SiStripGainsPCLWorker and SiStripGainsPCLHarvester) and in the decommissioned class (SiStripGainFromCalibTree) which will be kept active as a backup in the case of the PCL workflow failure.