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
GEMGeometryBuilder bugfix #35855
GEMGeometryBuilder bugfix #35855
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35855/26231
|
A new Pull Request was created by @jshlee (Jason Lee) for master. It involves the following packages:
@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob 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-b42cd8/19967/summary.html Comparison SummarySummary:
|
@jshlee , can you, please, explain what is the change for Run-3 GEM geometry? Or this is only for Phase-2? |
+1 |
@@ -524,8 +524,9 @@ void GEMGeometryBuilder::buildRegions(GEMGeometry& theGeometry, const std::vecto | |||
auto chamber = theGeometry.chamber(chId); | |||
if (!chamber) { | |||
edm::LogWarning("GEMGeometryBuilder") << "Missing chamber " << chId; | |||
} else { | |||
superChamber->add(chamber); |
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 @jshlee
I am confused a bit on the PR description and code changes. It seems the original code already protect non-exisitng chamber already. This PR seems to add existing chamber to super chamber. What will happen if you don't add chamber to superChamber (i.e. now)?
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.
the original code would just send the logwarning if the chamber is invalid and still add it to the superchamber.
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.
Ah, OK. I miss line 528 of the original code. Thanks.
type bug-fix |
+Upgrade This is a bug-fix PR, to include only existing GEM chamber to super chamber. |
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) |
+1 |
PR description:
PR validation:
tested with wf 34611.0