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
Run3-hcx303 Add a few cfi's to help processing with DDD or DD4Hep #34569
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34569/24112
|
A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master. It involves the following packages:
@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @kpedro88, @cmsbuild, @srimanob, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild Please test |
from Geometry.HcalCommonData.caloSimulationParameters_cff import * | ||
from Geometry.HcalCommonData.hcalhcalDBConstants_cff import * |
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.
Does hcalhcalDBConstants_cff
exist? The hcalhcal
part seems odd.
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.
Corrected
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34569/24113
|
Pull request #34569 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @kpedro88, @cmsbuild, @srimanob, @mdhildreth can you please check and sign again. |
from Geometry.HcalCommonData.hcalSimulationConstants_cfi import * | ||
from Geometry.HcalCommonData.caloSimulationParameters_cff import * | ||
from Geometry.HcalCommonData.hcalDDDSimConstants_cfi import * |
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.
Is the order of the cfi's important?
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.
No it is not.
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.
Then, unless my eyes are failing me, there is no actual change to this file, and it should be removed from the PR.
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.
What is the conclusion on this question? Thanks.
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 have the ordering to be similar in all cff files. That is why it is changed here.
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.
OK from my side. This is up to @cvuosalo.
Thanks.
@cmsbuild Please test |
-1 Failed Tests: UnitTests Unit TestsI found errors in the following unit tests: ---> test test2021Geometry had ERRORS ---> test test2026Geometry had ERRORS Comparison SummarySummary:
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0c2a2f/17147/summary.html Comparison SummarySummary:
|
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0c2a2f/17165/summary.html Comparison SummarySummary:
|
@civanch |
I don't see why it is not approved. There are serious bug fixes in a few cff's. |
Hi @bsunanda What is the bug that you are talking about? Can we not spot it on report (maybe I miss the bug report)? |
+Upgrade |
+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. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Add a few cfi's to help processing with DDD or DD4Hep
PR validation:
Use the standard workflows
if this PR is a backport please specify the original PR and why you need to backport that PR:
Nothing special