-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Diamond dqm #35454
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
Diamond dqm #35454
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35454/25610
|
|
A new Pull Request was created by @ChrisMisan (Christopher) for master. It involves the following packages:
@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Is there any reason to not join this PR with #35445 just submitted? |
Those PRs are independent, there is one common file(ctppsDQM_cff) but changes there are also independent. Thought it'd be cleaner to separate them. |
|
OK, but I do understand they do not need each other to run and see results. Thanks |
|
please test |
|
-1 Failed Tests: RelVals RelValsExpand to see more relval errors ... |
|
This PR was tested with GT 120X_dataRun2_v2, there's possibility relval uses "wrong" GT. |
|
This PR is in master (i.e. 12_1_X) so you should make sure a corresponding GT to 120X_dataRun2_v2 exists |
@jfernan2 From the log of 1330.0 I see the actual GT used is EDIT: other workflows use different autoCond GTs so we will have to review all of them I guess... |
|
This PR can be tested with Run2 data and right GT. PPS data are not in MC, just a reminder |
|
|
||
| _ctppsDQMOfflineSource = cms.Sequence( | ||
| ctppsPixelDQMOfflineSource | ||
| + ctppsDiamondDQMSource |
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.
@vavati this PR is modifying the _ctppsDQMOfflineSource to include CTPPSDiamondDQMSource.cc which is also modified here so that it requires LHCInfo conditions. So in order to test this PR new GTs will be needed. I don't see how you can test it without modifying the GT (even if you run on run2 data)
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.
My point is: if you have a single ctpps dqm sequence and you use it for both MC and Data you will have to provide all the proper tags in all GTs. If you don't care about ctpps in the MC sequence then you should probably split (or customize) the sequence in a different way for data and for MC.
I hope I understood the issue correctly. Maybe @jfernan2 can comment/suggest as well.
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.
I totally agree with @francescobrivio
We cannot approve a PR which is not going to run in production....
| # pixel | ||
| process.load('RecoPPS.Local.ctppsPixelLocalTracks_cfi') | ||
|
|
||
| process.load('Geometry.VeryForwardGeometry.geometryRPFromDD_2021_cfi') |
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.
Why are you using the Run 3 geometry read from the XML file? Was there a specific reason for this choice?
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.
Why are you using the Run 3 geometry read from the XML file? Was there a specific reason for this choice?
Yes, with run3 geometry dqm should produce empty plots for the second station so this is included for validation purposes.
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.
@malbouis
As you know the TAG for a partial Run3 geometry has been released only recently
@ChrisMisan, would you please add in the description how you tested this PR? |
|
Pull request #35454 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please check and sign again. |
|
please test |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17916c/19630/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Comparison SummarySummary:
|
|
+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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
Let wait to retest this on an IB which already contains #35445, likely this night 2300, so that we can disentangle the effects of this PR alone... |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17916c/19648/summary.html Comparison SummarySummary:
|
|
+1
|
Overview:
This PR introduces new diamond DQM module with following changes to the previous one:
Lumisection plots:
Following the discussion on lumisection plots it was decided to leave them commented out for the time being due to possible unforeseen issues when parallelized.
Testing

PR was validated by running test/diamond_dqm_test_cfg.py with conditions included in the file.
Memory performance:
This is memory consumption for cmsRun process running whole DQM Online.