-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Proper Clamping for CA Doublets Histogram #49282
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
Conversation
|
cms-bot internal usage |
|
type bug-fix |
|
enable gpu |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49282/46645 Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
e0e61c0 to
49b8b8a
Compare
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49282/46646 |
|
A new Pull Request was created by @AdrianoDee for master. It involves the following packages:
@jfernan2, @mandrenguyen, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Sorry, this PR changes a cout and a printf - so how is this affecting code executed in order to cure the crash? |
I guess (based on the PR description) this is the actual fix: - if (ind >= maxNumOfDoublets) {
+ if (ind >= maxNumOfDoublets or ind >= uint32_t(outerHitHisto->capacity())) { |
|
Ah yes, thanks! |
|
This PR is being supersed by #49286 right? |
Sort of, but I would anyway integrate this as is since it's a bug fix and it would be better to have it in quickly (for HI). |
|
+1 Size: This PR adds an extra 28KB to repository Comparison SummarySummary:
AMD_MI300X Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
AMD_W7900 Comparison SummarySummary:
NVIDIA_H100 Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
NVIDIA_L40S Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
NVIDIA_T4 Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
|
|
urgent |
|
+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. @sextonkennedy, @ftenchini, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR proposes a small fix for the crash reported in #49186. The crash there happens because we fill the
outerHitHistohisto beyond its capacity (that is defined asnHitsToCells = nHits * avgCellsPerHit). The check there is only done formaxDoublets. Now, technically,maxDoubletsandnHitsToCellsshould be the same (given the one-to-one correspondence between a doublet and its outer hit). Having two distinct variables is inherited from how the structures were defined before #47611 (oneOuterHitOfCellarray per hit). In the longer term, these two numbers should be merged. For the moment, I'm just adding a small fix to have it quickly integrated in15_1_Xfor HI operations.PR validation:
Reproducer in #49186 runs without error (and we get the warnings if
CA_WARNINGSis defined).