-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[13.0.X] Modernize TrackSplittingMonitor
and fix bug with filling ME-s
#41740
[13.0.X] Modernize TrackSplittingMonitor
and fix bug with filling ME-s
#41740
Conversation
Co-authored-by: Andrea Perrotta <[email protected]>
A new Pull Request was created by @mmusich (Marco Musich) for CMSSW_13_0_X. It involves the following packages:
@nothingface0, @emanueleusai, @cmsbuild, @pmandrik, @syuvivida, @tjavaid, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
type bug-fix |
@cmsbuild, please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e4a7e5/32728/summary.html Comparison SummarySummary:
|
ping @cms-sw/dqm-l2 |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_13_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_13_2_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
@mmusich when comparing the differences wrt to the master version of this PR I realize that there is a bug (not from this fix) in line
nRechitinBPIX1 is called twice, the second time instead of nRechitinBPIX2 .
This was "fixed" in master by removing the lines which define I think this should get fixed both in master and also here: could you please take care iof it? |
let's do it in another PR. |
this cannot possibly come from this PR. |
@cmsbuild, please test |
-1 Failed Tests: RelVals-INPUT RelVals-INPUT
Expand to see more relval errors ...
Comparison SummarySummary:
|
@cmsbuild, please test |
@mmusich those RelValInput errors are (annoyingly) rather frequent, and cannot depend on this PR. (They are not even present in the sibling PRs in master and 13_1_X). |
+1
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e4a7e5/32792/summary.html Comparison SummarySummary:
|
Please refrain from making condescending and inaccurate statements. This is a professional working environment, so please act accordingly. As a recommendation for the future, for truly urgent matters, please use the urgent label. [1] http://cds.cern.ch/record/2690506/files/Code%20of%20Conduct,%20English.pdf |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_13_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_13_2_X is complete. This pull request will be automatically merged. |
Answering on gitHub for the public record:
The statement was not condescending nor inaccurate. I was merely expressing the preference to introduce the further bug-fix in a second PR in order to avoid loosing the deadline for inclusion of the main fix in the data-taking release due to the subsystem (and cms-bot) signature has arrived after the deadline. This has unfortunately happened a number of times in my experience.
I appreciate that. This is true for most people in the collaboration. Although, precisely for this reason, we generally have more than one manager per software area. I count 7 CMSSW_L2 for DQM here. I am genuinely wondering why the load is not shared among the people that formally hold responsibility.
Please point me to which item of the code of conduct I have infringed.
I would have preferred to resort to that for case of real urgency. In this case it would have been a nice to have feature, that could easily enter the release given it's very limited consequences. But if that's what's required I'll do that in future. |
backport of #41690
PR description:
The main goal of this PR is to fix the filling of the residuals in the
TrackSplittingMonitor
DQM code. I noticed that in recent cosmic data these plots are almost all empty (see e.g.https://tinyurl.com/2q9tgomk). This was caused by a bug in the filling of theMonitorElements
which is fixed in this PR.I profit of it to:
fillDescriptions
method and use it in the related configurationsTrackSplitMonitor
from the Tracking DQM collisions setup as anyway there are no right input tracks to be fed to the module.PR validation:
Run successfully
runTheMatrix.py -l 138.1 -t 4 -j 8 --custom_command='-n 5000' --ibeos
and checked that the cosmic plots are all filled.If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
Verbatim backport of #41690 needed for 2023 data-taking.