-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Phase2-hgx132 Bug fix for HGCal post TDR geometry #24134
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24134/5823 |
A new Pull Request was created by @bsunanda for master. It involves the following packages: Geometry/HGCalCommonData @civanch, @Dr15Jones, @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. |
@bsunanda there were a few different bugs reported by @hatakeyamak:
Can you clarify which bugs should be fixed by this PR? |
Hopefully all 3. There is a small issue of virtual cells. It may indicate a small excursion beyond the limit because the cell may be partially contained and the center of the cell (whose position is returned) may be outside the limit. This can be almost a few mm. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@@ -897,14 +960,25 @@ void HGCalDDDConstants::waferFromPosition(const double x, const double y, | |||
auto itr = hgpar_->typesInLayers_.find(HGCalWaferIndex::waferIndex(layer,waferU,waferV)); | |||
celltype = ((itr == hgpar_->typesInLayers_.end()) ? 2 : | |||
hgpar_->waferTypeL_[itr->second]); | |||
if (debug) |
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.
@bsunanda - why do you need different debug modes:
https://github.com/cms-sw/cmssw/pull/24134/files#diff-ec8f6d9d7b440aeb0d87254f84cefbbbR821
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 only reason is to trace a rare error. Now that the error is detected and corrected for I can revert this if you want. I wanted the debug to be active only when needed.
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.
please, do. Thanks.
|
||
float x(0), y(0); | ||
int indx = HGCalWaferIndex::waferIndex(lay,waferU,waferV); | ||
auto itr = hgpar_->typesInLayers_.find(indx); | ||
int type = ((itr == hgpar_->typesInLayers_.end()) ? 2 : | ||
hgpar_->waferTypeL_[itr->second]); | ||
if (debug) |
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.
@bsunanda - please, use compile time debug
int kndx = cellV*100 + cellU; | ||
if (type == 0) { | ||
auto ktr = hgpar_->cellFineIndex_.find(kndx); | ||
if (ktr != hgpar_->cellFineIndex_.end()) { | ||
x = hgpar_->cellFineX_[ktr->second]; | ||
y = hgpar_->cellFineY_[ktr->second]; | ||
} | ||
if (debug) | ||
edm::LogVerbatim("HGCalGeom") << "Fine " << cellU << ":" << cellV << ":" |
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.
@bsunanda - please, use compile time debug
} else { | ||
auto ktr = hgpar_->cellCoarseIndex_.find(kndx); | ||
if (ktr != hgpar_->cellCoarseIndex_.end()) { | ||
x = hgpar_->cellCoarseX_[ktr->second]; | ||
y = hgpar_->cellCoarseY_[ktr->second]; | ||
} | ||
if (debug) |
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.
@bsunanda - please, use compile time debug
@@ -568,6 +582,9 @@ std::pair<float,float> HGCalDDDConstants::locateCell(int lay, int waferU, | |||
const auto & xy = waferPosition(waferU, waferV, reco); | |||
x += xy.first; | |||
y += xy.second; | |||
if (debug) |
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.
@bsunanda - please, use compile time debug
if ((std::abs(yloc-yc) <= rc) && (std::abs(xloc-xc) <= Rc) && | ||
((std::abs(xloc-xc) <= 0.5*Rc) || | ||
(std::abs(yloc-yc) <= sqrt3_*(Rc-std::abs(xloc-xc))))) { | ||
if (debug) |
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.
@bsunanda - please, use compile time debug
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.
Ianna - a compile time debug will print all occurrences. This allows only when there is some special investigation.
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.
Do you need to do special investigation in production? Because the check will be made every time even if you do not need any printout.
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 am leaving selective debug statement so that I can easily revive partial debug. Hope this is oK with you
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@bsunanda I would prefer to get confirmation from @hatakeyamak that this indeed fixes the bugs that were observed |
I am checking this today. Try to report here asap. |
@bsunanda - are differences in Reco comparison results justified: 3 differences found in the comparisons? |
@ianna these are spurious differences from non-reproducibility in a PU workflow |
+1 |
+1 |
My check summary is here: |
+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 now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
Correct cell assignments for virtual cells
Also include some debug facility during SIM and DIGI steps