-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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-TB42 Correct numbering for AHCal so that |row|/|column| can exceed 10 #27708
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27708/11327
|
A new Pull Request was created by @bsunanda for master. It involves the following packages: Geometry/HGCalCommonData @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild Please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
+1 @kpedro88 this looks relatively simple to me |
int incol = ((touch->GetReplicaNumber(0)) % 100); | ||
int inrow = ((touch->GetReplicaNumber(0)) / 100) % 100; | ||
int jncol = ((touch->GetReplicaNumber(0)) / 10000) % 10; | ||
int jnrow = ((touch->GetReplicaNumber(0)) / 100000) % 10; |
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.
These factors of 10 are magic numbers. The values in DDAHcalModuleAlgo.cc
must match those in AHCalSD.cc
for this code to work right. The best way to ensure that is for both files to use the same constants. Since this PR is bug fix, this issue doesn't need to be fixed now, but it would be good to clean it up eventually.
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.
@cvuosalo I agree, on the other side this was the situation even before, I believe the adjustment using named constants can be factorized
merge |
+upgrade |
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 be automatically merged. |
PR description:
The earlier code was restricting row-column number for AHCal cell to be within -9 to +9. In fact it has to be between -12 to +12. The code is corrected to allow that
PR validation:
Tested with scripts for TestBeam simulation
if this PR is a backport please specify the original PR:
Nothing special