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 GEMGeometryBuilder Run3 modifier #38666
Run3 GEMGeometryBuilder Run3 modifier #38666
Conversation
@hyunyong please make it clear in the PR description that you want this in 12_4_X... |
type gem,new-feature |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38666/30956
|
A new Pull Request was created by @hyunyong for master. It involves the following packages:
@civanch, @Dr15Jones, @bsunanda, @makortel, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild Please test |
This will fail w/o a new GT |
-1 Failed Tests: RelVals RelVals-INPUT AddOn RelVals
Expand to see more relval errors ...RelVals-INPUT
Expand to see more relval errors ...
AddOn Tests
Expand to see more addon errors ... |
|
||
from Configuration.Eras.Modifier_run3_GEM_cff import run3_GEM | ||
|
||
run3_GEM.toModify(GEMGeometryESModule, applyAlignment = True) |
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.
Done we also need
phase2_GEM.toModify(GEMGeometryESModule, applyAlignment = True)
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.
phase2 GEM has GE2/1 and GE0, so it needs a different geometry tag.
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.
After private discussion offline between PR author and @cms-sw/alca-l2 it was decided to postpone the update of phase2 to a later moment.
test parameters: |
@cmsbuild please test |
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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
urgent
|
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-385255/26749/summary.html Comparison SummarySummary:
|
@hyunyong Just to confirm that the differences shown in wf 138.5 is expected or not?: |
|
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-385255/26808/summary.html Comparison SummarySummary:
|
@hyunyong @jshlee @watson-ij @quark2 the differences shown in wf 138.5 is expected or not?: |
Hi @qliphy, Sorry for my late reply. I've reproduced the compared plots, some of which are the follows: The changes are slightly different, which should be due to the alignment. All other 3 plots have similar slight shifts. Note that this PR is for the alignment, and these plots are from real data (the wf 138.5.) Best regards, |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-385255/26935/summary.html Comparison SummarySummary:
|
+1 |
@jshlee @watson-ij
GEM Run3 modifier setup for the alignment.
PR description:
Update the GEM geometry builder to use alignment tags.
Need to backport to 12_4_X
GEM doesn't have ideal geometry cff for digi.
Do we need to add ideal geometry cff and update configurations?
Some configurations use GEM default geometry builder as ideal geometry but they don't use the Run3 modifier, so it looks fine but I need to get a confirmation.